[Ocfs2-tools-devel] [PATCH 4/5] ocfs2-tools: add xattr_uuid_hash in libocfs2

[Joel Becker]

On Sun, Oct 26, 2008 at 10:14:53AM +0800, tristan.ye wrote:
> On Fri, 2008-10-24 at 17:58 +0800, Coly Li wrote:
> > 
> > Tiger Yang Wrote:
> > > this function use to calculate uuid hash, this hash value will be
> > > used as random seed to calculate xattr hash.

<snip>

> > > +uint32_t xattr_uuid_hash(unsigned char *uuid)
> > > +{
> > > +	uint32_t i, hash = 0;
> > > +
> > > +	for (i = 0; i < OCFS2_VOL_UUID_LEN; i++) {
> > > +		hash = (hash << OCFS2_HASH_SHIFT) ^
> > > +			(hash >> (8*sizeof(hash) - OCFS2_HASH_SHIFT)) ^
> > > +			*uuid++;
> > Maybe *(uuid ++) is more clear ?
> 
> seems it was clear enough here since the '++' operator has higher
> priority than '*':)

	But the '++' happens after, remember.  Correctness is one level.
Readability is another.  Now, '*uuid++' is an idiom, but an ugly one.  It
means "dereference uuid, use that value, then increment uuid".  I'm pretty
sure that's what Tiger meant.  But Coly didn't get that, so he suggested
'*(uuid++)', which means "increment uuid, then dreference it, using the
new value".  Also the same as *++uuid.  So Coly's suggestion ends up as
a bug.  The point is not to pick on Coly; this is why certain constructs
are considered unreadable.  I went and looked up the precedence table to
be sure I was speaking correctly ;-)
	If we want a clear version of the function, why not:

+uint32_t xattr_uuid_hash(unsigned char *uuid)
+{
+	uint32_t i, hash = 0;
+
+	for (i = 0; i < OCFS2_VOL_UUID_LEN; i++) {
+		hash = (hash << OCFS2_HASH_SHIFT) ^
+			(hash >> (8*sizeof(hash) - OCFS2_HASH_SHIFT)) ^
+			uuid[i];

Joel

-- 

"You must remember this:
 A kiss is just a kiss,
 A sigh is just a sigh.
 The fundamental rules apply
 As time goes by."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127