On Mon, Oct 10, 2011 at 12:23:04PM +0100, Måns Rullgård wrote: > Janne Grunau <janne-li...@jannau.net> writes: > > On Sun, Oct 09, 2011 at 01:07:09PM +0100, Mans Rullgard wrote: > >> Keeping byte values read from the file as unsigned is consistent > >> with how they are subsequently used and avoids an undefined left > >> shift by 24 when bit 7 is set. > >> > >> --- a/libavformat/avidec.c > >> +++ b/libavformat/avidec.c > >> @@ -860,7 +861,7 @@ start_sync: > >> > >> - if(i + (uint64_t)size > avi->fsize || d[0]<0) > >> + if(i + (uint64_t)size > avi->fsize || d[0] > 127) > >> continue; > > > > looks reasonable, Alex might want to comment though. > > > > please fix the ugly "memset(d, -1, sizeof(int)*8);" while you're at it > > I refuse. I thought we agreed to have no more "patch ok if you fix > everything else first" reviews. Holding a fix hostage demanding > additional unrelated things is insane.
I agree that patches should not be held hostage, but there should nonetheless be a way to comment on unrelated things noticed during patch review. This has come up before, so we should find a way to address this. I would suggest prefixing extra suggestions with "nit:" or "unrelated:" or something similar. The patch sender or anybody else can then decide to implement such optional requests at their discretion. Diego _______________________________________________ libav-devel mailing list libav-devel@libav.org https://lists.libav.org/mailman/listinfo/libav-devel
