subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Sperling <s...@elego.de>
Subject Re: svn commit: r1042294 - /subversion/trunk/subversion/libsvn_subr/checksum.c
Date Sun, 05 Dec 2010 11:21:38 GMT
On Sun, Dec 05, 2010 at 05:14:20AM -0600, Peter Samuelson wrote:
> 
> [Stefan Sperling]
> > > -      is_zeros |= (*checksum)->digest[i];
> > > +      is_nonzero |= ((char *)(*checksum)->digest)[i] = x1 << 4 |
x2;
> > 
> > At the very least, this needs parenthesis unless you want everyone
> > to pull out their copy of K&R to check operator precedence rules.
> 
> Can do.  x1 << 4 | x2 didn't seem too ambiguous to me, but I can see
> how it might be.
> 
> > Can we do one assignment per line instead? Maybe that's easier to parse.
> 
> Hmmm.
> 
>       ((char *)(*checksum)->digest)[i] = x1 << 4 | x2;
>       is_nonzero |= ((char *)(*checksum)->digest)[i];
> 
> ...The repeated expression is complex, so you have to visually verify
> that it is indeed identical.  That seems harder, to me.  Or did you
> mean:
> 
>       is_nonzero |= 
>         ((char *)(*checksum)->digest)[i] = x1 << 4 | x2;
> 
> ...Which doesn't seem much easier to parse either.

What about something like this (pseudo code):

      x = x1 << 4 | x2;
      ((char *)(*checksum)->digest)[i] = x;
      is_nonzero |= x;

Stefan

Mime
View raw message