[FFmpeg-devel] remove int readers

[ods15 at ods15.dyndns.org]

On Thu, Jun 14, 2007 at 11:07:12AM +0200, Baptiste Coudurier wrote:
> Hi guys,
> 
> Michael Niedermayer wrote:
> > Hi
> > 
> > On Thu, Jun 14, 2007 at 09:59:32AM +0200, Reimar Doeffinger wrote:
> >> Hello,
> >> On Thu, Jun 14, 2007 at 08:21:08AM +0100, M??ns Rullg??rd wrote:
> >>> "Ronald S. Bultje" <rsbultje at gmail.com> writes:
> >>>> On 6/14/07, mark cox <melbournemark+ffmpeg at gmail.com> wrote:
> >>>>> -    seq  = (buf[2] << 8) | buf[3];
> >>>>> +    seq  = AV_RB16(buf + 2);
> >>>>>
> >>>>> Why have you changed all the indexes from [] to +. [] are preferred
> >>>>> because they avoid compiler warnings and are easier to read IMHO.
> >>>> Consistency with the rest of the code. I prefer &buf[2] also, but I try to
> >>>> conform to what's already there.
> >>> &buf[2] is IMO far harder to read than buf + 2, and it's ugly to
> >>> boot.
> >> I don't think we will ever get a common opinion on this, I usually prefer
> >> &buf[2] syntax and find buf + 2 ugly ;-)
> > 
> > i am on mans side this time :)
> > also +2 is 2 chars &[2] is 4 so its simpler ;)
> > 
> > anyway, we should be carefull to seperating such bikeshed issues from real
> > clear readability issues, i dont want ffmpeg to become known for rejecting
> > patches because of such style issues ...
> > 
> 
> Im with Mans and Michael, sorry Reimar ;)

I'm not on any side, but would like to note a really bad bug I had in tcc 
once when trying to compile ffmpeg, because of this issue:

int8_t buf[4] = {5,1,0,0};
int a = *(int32_t*)(buf + 0); // gets value 261
int b = *(int32_t*)&buf[0];   // gets value 5 :(

I had to chase this down all the way in dsputil.c, by seperating it to 2 
files, one compiled by gcc and one by tcc, moving functions around one by 
one... :)

This is ofcourse entirely irrelevant to style, just found it appropiate to 
the discussion...

BTW, I did finally manage to compile it after heavy modifications. the 
result was over 2 times slower than gcc -O0 with all ASM disabled. (Both 
had ASM disabled)

> I also agree with Michael that we should not reject patches because of
> that, though maintainer should be free to prefer one syntax IMHO.

Agreed. :)

- ods15