On Tue, Sep 06, 2011 at 03:06:26AM -0400, Dustin Brody wrote:
> 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.
I could not resist doing this properly. Please review the patch that is
about to arrive in this thread. Filtering out whitespace changes should
make this easy. Applying the patch and looking at it with "git log -p -w"
is one possibility.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
