On Tue, 2 Sep 2014 16:34:58 -0700 Peter Kasting <pkast...@google.com> wrote:
> On Tue, Sep 2, 2014 at 3:19 PM, wm4 <nfx...@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
