[FFmpeg-devel] [PATCH] Fix MSVC warnings about possible value truncation.

[wm4]

On Tue, 2 Sep 2014 16:34:58 -0700
Peter Kasting <pkasting at google.com> wrote:

> On Tue, Sep 2, 2014 at 3:19 PM, wm4 <nfxjfg at googlemail.com> wrote:
> 
> > Here's why patches like these are bad: they're
> > HUGE. Reviewing them takes a lot of time, and the result is
> > questionable. How are we going to do v2 of this patch? And v3 etc.?
> > Probably not at all. It's just too monolithic. It's like reviewing a
> > phone book.
> >
> > For the next time, I'd suggest putting every real fix (or attempt of a
> > fix) into a separate patch - even if it results in dozens of patches.
> > At least this makes it easier to see what patch got rejected, what got
> > accepted, and what needs further work.
> 
> 
> I'm perfectly happy to do that -- indeed, I tried to say in my very first
> email that every file in this patch is basically independent, and I could
> break the patch down into chunks of whatever size people deemed reasonable,
> even one per file (or, potentially, even smaller).  I just wasn't sure what
> would make sense.  Telling me "please come back with a series of extremely
> tiny patches" is fine feedback.  What I'll do with that feedback, going
> forward, is try and split this patch into small pieces that account for the
> various issues you raise separately.  Hopefully that's reasonable.

Yes. For all the semi-automatic changes it's probably best to group
them anyway. Well, apply common-sense.

> 
> Another problem: this mixes hiding compiler warnings and trying to fix
> > real problems. And in many cases, hiding real problems. Just plastering
> > everything with "(type)expr" won't help much, especially not if "type"
> > is signed, because it doesn't change anything about the legality of an
> > overflow.
> 
> 
> I tried not to "just plaster things with casts", but as I said, I was
> counting on review feedback to help me understand where changes would be
> fixing real problems or hiding real problems, as opposed to just silencing
> warnings.  The ultimate goal, of both this patch and of enabling these
> sorts of warnings in general, is to find and fix the real problems, hide no
> problems, and hopefully not cost too much in terms of additional code noise
> to "silence" non-problem cases.

That is perfectly fine. I just wanted to express my opinion that
silencing compiler warnings for the sake of silencing compiler warnings
by adding casts is not a good idea. (And in general, compiler warnings
that produce large amounts of false positives are a bad idea, because
they just drain developer time, or encourage bad practices.)

> 
> Thanks for the review comments, to both of you who provided some.  I will
> go try to address them and return with smaller patches.
> 
> PK
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel