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.
(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....
(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.
Created attachment 250394 [details] Patch
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.
(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 :)
Comment on attachment 250394 [details] Patch Clearing flags on attachment: 250394 Committed r182676: <http://trac.webkit.org/changeset/182676>
All reviewed patches have been landed. Closing bug.
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.
I see now that Michael and Ben had this exact conversation yesterday!