Re: svn commit: r1042294 - /subversion/trunk/subversion/libsvn_subr/checksum.c

[Daniel Shahaf <d....@daniel.shahaf.name>]

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

Re: svn commit: r1042294 - /subversion/trunk/subversion/libsvn_subr/checksum.c

[Peter Samuelson <pe...@p12n.org>]

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

r1042319.  I left both lines as (x1 << 4) | x2, even though is_nonzero
doesn't need the << 4, so that the compiler will only calculate a
single expression.