Bug 143457 – Fix -Wparentheses warning with GCC 5 in SaturatedArithmetic.h

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Michael Catanzaro]

When building with GCC 5, I can't see any real warnings, because this buddy gets printed 3000 times [1] during compilation:

../../Source/WTF/wtf/SaturatedArithmetic.h: In function ‘bool signedAddOverflows(int32_t, int32_t, int32_t&)’:
../../Source/WTF/wtf/SaturatedArithmetic.h:49:29: warning: suggest parentheses around operand of ‘!’ or change ‘&’ to ‘&&’ or ‘!’ to ‘~’ [-Wparentheses]
     return !((ua ^ ub) >> 31) & (uresult ^ ua) >> 31;
                             ^

Clang has had this same warning for a while. The warning is spurious [2], but it's a good warning so let's placate it rather than disable. Two solutions:

Add extra parentheses: return (!((ua ^ ub) >> 31)) & (uresult ^ ua) >> 31;

Note that is equivalent to the original code [3].

Another option is to use &&:

!((ua ^ ub) >> 31) && (uresult ^ ua) >> 31;

That looks better to me, since there are fewer parentheses and it avoids the confusing/unnecessary [3] mask.

[1] I just made that up, but it doesn't feel like an unreasonable guess.
[2] & and && both have the same precedence relative to !
[3] Provided I've squinted at this long enough.

[Michael Catanzaro]

(In reply to comment #0)
> Another option is to use &&:
> 
> !((ua ^ ub) >> 31) && (uresult ^ ua) >> 31;

Didn't squint hard enough; that needs more parentheses. The choices are:

(!((ua ^ ub) >> 31)) & (uresult ^ ua) >> 31;

or

!((ua ^ ub) >> 31) && ((uresult ^ ua) >> 31);

I think I prefer the later.



For subtraction, we currently have this:

(ua ^ ub) >> 31 & (uresult ^ ua) >> 31

If we use && for addition, we'd want to use && there as well, so it would become:

((ua ^ ub) >> 31) && ((uresult ^ ua) >> 31)

That seems a bit clearer....

[Michael Catanzaro]

(In reply to comment #1) 
> Didn't squint hard enough; that needs more parentheses.

Squinted too hard. >> has higher precedence than both & and && so the extra parentheses are not needed. Comment #0 is correct.

[Michael Catanzaro]

Created attachment 250394 [details]
Patch

[Michael Catanzaro]

Note, I only tested the change in GCC, so I'm not completely sure this removes the warning if you use Clang, but I guess it does.

If this causes a performance regression we should just add extra parentheses instead.

[Benjamin Poulain]

(In reply to comment #4)
> Note, I only tested the change in GCC, so I'm not completely sure this
> removes the warning if you use Clang, but I guess it does.
> 
> If this causes a performance regression we should just add extra parentheses
> instead.

If a compiler has trouble with something this simple, we should fix the compiler :)

[WebKit Commit Bot]

Comment on attachment 250394 [details]
Patch

Clearing flags on attachment: 250394

Committed r182676: <http://trac.webkit.org/changeset/182676>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 250394 [details]
Patch

Maybe & rather than && is what we wanted, but with the additional parentheses to make sure the shifts happen first? I guess a sufficiently high quality compiler would generate the same code for both.

[Darin Adler]

I see now that Michael and Ben had this exact conversation yesterday!