On 9/6/2011 2:57 AM, Diego Biurrun wrote:
> On Tue, Sep 06, 2011 at 02:17:48AM -0400, Dustin Brody wrote:
>> ---
>> libavcodec/mpeg12.c | 176
>> +++++++++++++++++++++++++-------------------------
>> 1 files changed, 88 insertions(+), 88 deletions(-)
>
> LGTM, but ...
>
>> --- a/libavcodec/mpeg12.c
>> +++ b/libavcodec/mpeg12.c
>> @@ -91,16 +91,16 @@ static void init_2d_vlc_rl(RLTable *rl)
>>
>> if(len==0){ // illegal code
>> run= 65;
>> - level= MAX_LEVEL;
>> + level = MAX_LEVEL;
>> }else if(len<0){ //more bits needed
>> run= 0;
>> - level= code;
>> + level = code;
>> }else{
>> if(code==rl->n){ //esc
>> run= 65;
>
> ... I do wonder why you change some lines and not others, cf. the
> three "run=" assignments above, more below.
>
>> @@ -110,12 +110,12 @@ static void init_2d_vlc_rl(RLTable *rl)
>> - rl->rl_vlc[0][i].len= len;
>> - rl->rl_vlc[0][i].level= level;
>> - rl->rl_vlc[0][i].run= run;
>> + rl->rl_vlc[0][i].len = len;
>> + rl->rl_vlc[0][i].level = level;
>> + rl->rl_vlc[0][i].run = run;
>
> And in places like this one you could align the '=' while you're at it.
Oh, because I used a sed one-liner that looks for
(c-identifier)(equals)(space)(c-identifier). I didn't go through the
file and change 88 lines manually.
This means that the first kind of thing is easy to do - "run= 0"
just lacks a C identifier in the RHS, but that's just a matter of
changing the RHS pattern.
The latter alignment, well, that's a change that has to be
consistent across a quasi-arbitrarily defined region of code,
perhaps easy for a human to spot, but not so much for sed. There's
really no 'while I'm at it' going on in such cases.
I focus on being conservative enough not to screw up existing
formatting or changing semantics.
-Dustin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
