On Wed, Nov 23, 2016 at 09:40:13AM +0100, Anton Khirnov wrote: > Quoting Diego Biurrun (2016-11-14 16:01:43) > > On Mon, Nov 14, 2016 at 12:11:47PM +0100, Anton Khirnov wrote: > > > Quoting Diego Biurrun (2016-11-14 11:58:34) > > > > On Mon, Nov 14, 2016 at 11:40:22AM +0100, Anton Khirnov wrote: > > > > > Quoting Diego Biurrun (2016-11-14 10:27:32) > > > > > > On Mon, Nov 14, 2016 at 10:12:02AM +0100, Anton Khirnov wrote: > > > > > > > Quoting Diego Biurrun (2016-11-14 10:03:43) > > > > > > > > On Mon, Nov 14, 2016 at 09:20:12AM +0100, Anton Khirnov wrote: > > > > > > > > > Quoting Diego Biurrun (2016-11-14 09:05:33) > > > > > > > > > > Fixes a number of warnings of the type > > > > > > > > > > libavutil/hmac.c:61:21: warning: assignment from > > > > > > > > > > incompatible pointer type > > > > > > > > > > > > > > > > > > No, this does not "fix" anything. It hides real warnings > > > > > > > > > caused by real > > > > > > > > > problems. I really wish you stopped fixing warnings and > > > > > > > > > perhaps fixed > > > > > > > > > the bugs that cause those warnings instead. > > > > > > > > > > > > > > > > Interestingly, this patch is from Kostya. I'm open to your > > > > > > > > suggestion > > > > > > > > of the "real fix". > > > > > > > > > > > > > > What does it matter who is it from? It is wrong. The compiler > > > > > > > prints a > > > > > > > warning because you're calling a function using a wrong signature > > > > > > > (signed int vs unsigned). Strictly speaking, this is UB. The only > > > > > > > thing > > > > > > > this patch does is shut up the warning, but the problem is still > > > > > > > there - > > > > > > > you are still calling a function using a wrong signature. > > > > > > > > > > > > No, the signed vs. unsigned thing is not the issue and does not > > > > > > affect the > > > > > > warning(s). > > > > > > > > > > It may not be the _only_ reason for the warnings, but it is certainly > > > > > one of the reasons. > > > > > > > > You always need a cast to assign non-identical function pointers. > > > > > > > > What is it that you suggest should be done? Change the API for sha and > > > > md5 take unsigned or size_t as type for the length parameter? We use > > > > nt as type for sizes in 99% of all cases throughout the codebase. > > > > > > MD5 already takes int, it's just SHA that's unsigned. Ideally all of > > > those should be size_t, so breaking API is one possibility. > > > > I've been advocating for size_t for sizes, but I have to admit that it > > feels a tad arbitrary to require size_t now given the amount of int-typed > > size variables that we have everywhere and that you yourself use even in > > current code. > > Got to start somewhere. I think it's perfectly fine to require proper > types in all new code (and yes I admit I forget about this occasionally > and use ints when I shouldn't - that's what review is for).
You shall be hearing from me more often now.. > > > Another, non-breaking one, would be to add wrappers to hmac.c, similarly > > > to what it already does for sha init. > > > > That would be pointless indirection IMO. Notice that the sha init wrappers > > pass an extra parameter of a fixed size. > > Not pointless -- it would do the signed-to-unsigned conversion and thus > fix the problem properly instead of just hiding the warning about it. The warning is not about the signed-to-unsigned conversion. An explicit assignment, i.e. cast, is needed in either case. Change the types locally and compile if you don't want to take my word for it. Diego _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
