subversion-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Shahaf <...@daniel.shahaf.name>
Subject Re: svn commit: r1042294 - /subversion/trunk/subversion/libsvn_subr/checksum.c
Date Sun, 05 Dec 2010 11:22:50 GMT
Peter Samuelson wrote on Sun, Dec 05, 2010 at 05:14:20 -0600:
> 
> [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;
> 

    char *digest = (char *) checksum->digest;
    for (i = 0; ...)
      {
      	is_nonzero |= (x1 | x2);
      	digest[i] = (x1 << 4) | x2;
      }

> ...Which doesn't seem much easier to parse either.
> 
> Peter

Mime
View raw message