551736 - "nsSMILCompositor.cpp:56: warning: suggest parentheses around + or - inside shift"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Just noticed this compile warning go by in my compile-spew:
> nsSMILCompositor.cpp:56: warning: suggest parentheses around + or - inside shift

That refers to this chunk:
50 /*static*/ PLDHashNumber
51 nsSMILCompositor::HashKey(KeyTypePointer aKey)
52 {
53   // Combine the 3 values into one numeric value, which will be hashed
54   return NS_PTR_TO_UINT32(aKey->mElement.get()) >> 4 +
55     NS_PTR_TO_INT32(aKey->mAttributeName.get()) +
56     (aKey->mIsCSS ? 1 : 0);
57 }
58 

I think we need parens around the " XXX >> 4 " there. (rather than around the + as the warning suggests...)

The fact that it suggests adding parens around the "+" makes me think the operator-precedence is currently such that we're using the latter 3/4 of that statement all as the amount to be shifted by... If so, that's broken -- but I guess at worst it just means our hash function isn't currently as good as it could be.

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

This chunk of code is from bug 534136, btw.

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Just talked to sicking -- we should actually be right-shifting by 2, not 4.  (If we don't shift at all, we'd always have 0's in the two lowest bits, since we're expecting those pointers to 4-byte-word-aligned.  But we don't need to shift by more than 2.)

Also: we should be using UINT32 (not INT32) on line 55, since that's what PLDHashNumber is typedef'd to.

Comment 3

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: fix: add parens, shift by 2, & use UINT

Comment 4

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Landed: http://hg.mozilla.org/mozilla-central/rev/8023fbbd3a70