[FFmpeg-devel] [PATCH] Bugfix: decoding 8- and 24-bit FLAC files

[Benoit Fouet]

Michael Donaghy wrote:
> On Tuesday 12 June 2007 12:56:08 Benoit Fouet wrote:
>
>   
>>>>>>> +                *(samples++) = (left  << (24 - s->bps)) >> 8;\
>>>>>>> +                *(samples++) = (right << (24 - s->bps)) >> 8;\
>>>>>>>               
>>>>>> i think this code lacks paretheses
>>>>>>             
>>>>> I think it has too many.
>>>>>           
>>>> ok, what i'd write would be:
>>>> *samples++ = ((left) << 24 - s->bps) >> 8;\
>>>>
>>>> is that ok ?
>>>>         
>>> iam fine with it though maybe we should keep the one around 24 - s->bps
>>> but thats just a slight personal preferance ...
>>>       
>> ok, then i'll just add the one around "left" and "right" if it's ok
>> and maybe remove all parentheses around "samples"
>>
>>     
> I wrote it the way I prefer to parenthesise it; I feel that *(samples++) makes 
> it more clear that it is the pointer rather than the target which is being 
> incremented, and likewise left << (24 - s->bps) makes it clear this is not 
> doing (left << 24) - s->bps, whereas ((left)  << (24 - s->bps)) seems no less 
> ambiguous than (left  << (24 - s->bps)) to my eyes. But shrug, I care far 
> less about the parenthesising than I do about the ability to play 24-bit 
> flacs.
>
>   

the only things that annoy me is the latter...
((left) << (24 - s->bps)) is not the same as (left << (24 - s->bps))
when we talk about macros...

there are operators with a priority inferior to <<, and that could
possibly lead to problems, even though the way the macro is used in the
code does not raise those problems...
furthemore, the original code had the parenthesis aroud "left" and
"right", and i keep on thinking they should have been kept.

Ben
-- 
Purple Labs S.A.
www.purplelabs.com