143457 2015-04-06 15:53:44 -0700 Fix -Wparentheses warning with GCC 5 in SaturatedArithmetic.h 2015-04-13 09:02:54 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) PC Linux RESOLVED FIXED P2 Minor --- 1 mcatanzaro mcatanzaro benjamin cmarcelo commit-queue darin mcatanzaro zan oldest_to_newest 1083190 0 mcatanzaro 2015-04-06 15:53:44 -0700 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. 1083194 1 mcatanzaro 2015-04-06 16:08:48 -0700 (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.... 1083296 2 mcatanzaro 2015-04-07 05:47:44 -0700 (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. 1083772 3 250394 mcatanzaro 2015-04-08 16:54:58 -0700 Created attachment 250394 Patch 1083774 4 mcatanzaro 2015-04-08 16:56:21 -0700 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. 1084708 5 benjamin 2015-04-12 17:56:17 -0700 (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 :) 1084711 6 250394 commit-queue 2015-04-12 18:30:59 -0700 Comment on attachment 250394 Patch Clearing flags on attachment: 250394 Committed r182676: <http://trac.webkit.org/changeset/182676> 1084712 7 commit-queue 2015-04-12 18:31:03 -0700 All reviewed patches have been landed. Closing bug. 1084816 8 250394 darin 2015-04-13 09:01:51 -0700 Comment on attachment 250394 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. 1084817 9 darin 2015-04-13 09:02:54 -0700 I see now that Michael and Ben had this exact conversation yesterday! 250394 2015-04-08 16:54:58 -0700 2015-04-12 18:30:59 -0700 Patch bug-143457-20150408185407.patch text/plain 2147 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMTgyNTY5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGIwZGU0NDMzNGY3Y2RlMGU2NDQxYmZm NTY2YWViYjViMjk3Y2Y4ZTMuLjM3NTdlYjc1Y2ZkZjA1ZGM3NDBhNzA4ZWMwZWNlYzg4MjZlNzk0 ZGQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTUtMDQtMDggIE1pY2hhZWwgQ2F0YW56YXJvICA8 bWNhdGFuemFyb0BpZ2FsaWEuY29tPgorCisgICAgICAgIEZpeCAtV3BhcmVudGhlc2VzIHdhcm5p bmcgd2l0aCBHQ0MgNSBpbiBTYXR1cmF0ZWRBcml0aG1ldGljLmgKKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0MzQ1NworCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRlc3RlZCBieSBXVEYuU2F0dXJhdGVkQXJp dGhtZXRpY0FkZGl0aW9uIGFuZCBXVEYuU2F0dXJhdGVkQXJpdGhtZXRpY1N1YnRyYWN0aW9uLgor CisgICAgICAgICogd3RmL1NhdHVyYXRlZEFyaXRobWV0aWMuaDoKKyAgICAgICAgKHNpZ25lZEFk ZE92ZXJmbG93cyk6IFVzZSAmJiBpbnN0ZWFkIG9mICYgdG8gYXZvaWQgdHJpZ2dlcmluZyAtV3Bh cmVudGhlc2VzIGluIG5ld2VyCisgICAgICAgIHZlcnNpb25zIG9mIEdDQyBhbmQgQ2xhbmcsIGFu ZCB0byBpbXByb3ZlIHRoZSBjbGFyaXR5IG9mIHRoZSBmdW5jdGlvbi4KKyAgICAgICAgKHNpZ25l ZFN1YnRyYWN0T3ZlcmZsb3dzKTogQ2hhbmdlZCBjb3JyZXNwb25kaW5nbHksIGFsdGhvdWdoIHRo ZXJlIHdhcyBubyB3YXJuaW5nIGhlcmUuCisKIDIwMTUtMDQtMDggIEFsZXggQ2hyaXN0ZW5zZW4g IDxhY2hyaXN0ZW5zZW5Ad2Via2l0Lm9yZz4gYW5kIFBhdHJpY2sgR2Fuc3RlcmVyICA8cGFyb2dh QHdlYmtpdC5vcmc+CiAKICAgICAgICAgQWRkIENNYWtlIGJ1aWxkIHN5c3RlbSBmb3IgV2luQ2Fp cm8gcG9ydC4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvd3RmL1NhdHVyYXRlZEFyaXRobWV0aWMu aCBiL1NvdXJjZS9XVEYvd3RmL1NhdHVyYXRlZEFyaXRobWV0aWMuaAppbmRleCAyYzc5MjhiZGQ3 ZTlhMTRhYWNlNDkxMDE1MjZiMTcyNDg5YjMwYzNhLi44N2E1NGM5NzYzNDE4NDZkYzE2Y2NkYzhm NDk4YmEwZjFhZGI1NzVjIDEwMDY0NAotLS0gYS9Tb3VyY2UvV1RGL3d0Zi9TYXR1cmF0ZWRBcml0 aG1ldGljLmgKKysrIGIvU291cmNlL1dURi93dGYvU2F0dXJhdGVkQXJpdGhtZXRpYy5oCkBAIC00 Niw3ICs0Niw3IEBAIGlubGluZSBib29sIHNpZ25lZEFkZE92ZXJmbG93cyhpbnQzMl90IGEsIGlu dDMyX3QgYiwgaW50MzJfdCYgcmVzdWx0KQogCiAgICAgLy8gQ2FuIG9ubHkgb3ZlcmZsb3cgaWYg dGhlIHNpZ25lZCBiaXQgb2YgdGhlIHR3byB2YWx1ZXMgbWF0Y2guIElmIHRoZSBzaWduZWQKICAg ICAvLyBiaXQgb2YgdGhlIHJlc3VsdCBhbmQgb25lIG9mIHRoZSB2YWx1ZXMgZGlmZmVyIGl0IGRp ZCBvdmVyZmxvdy4KLSAgICByZXR1cm4gISgodWEgXiB1YikgPj4gMzEpICYgKHVyZXN1bHQgXiB1 YSkgPj4gMzE7CisgICAgcmV0dXJuICEoKHVhIF4gdWIpID4+IDMxKSAmJiAodXJlc3VsdCBeIHVh KSA+PiAzMTsKIH0KIAogaW5saW5lIGludDMyX3Qgc2F0dXJhdGVkQWRkaXRpb24oaW50MzJfdCBh LCBpbnQzMl90IGIpCkBAIC03NCw3ICs3NCw3IEBAIGlubGluZSBib29sIHNpZ25lZFN1YnRyYWN0 T3ZlcmZsb3dzKGludDMyX3QgYSwgaW50MzJfdCBiLCBpbnQzMl90JiByZXN1bHQpCiAKICAgICAv LyBDYW4gb25seSBvdmVyZmxvdyBpZiB0aGUgc2lnbmVkIGJpdCBvZiB0aGUgdHdvIHZhbHVlcyBk byBub3QgbWF0Y2guIElmIHRoZQogICAgIC8vIHNpZ25lZCBiaXQgb2YgdGhlIHJlc3VsdCBhbmQg dGhlIGZpcnN0IHZhbHVlIGRpZmZlciBpdCBkaWQgb3ZlcmZsb3cuCi0gICAgcmV0dXJuICh1YSBe IHViKSA+PiAzMSAmICh1cmVzdWx0IF4gdWEpID4+IDMxOworICAgIHJldHVybiAodWEgXiB1Yikg Pj4gMzEgJiYgKHVyZXN1bHQgXiB1YSkgPj4gMzE7CiB9CiAKIGlubGluZSBpbnQzMl90IHNhdHVy YXRlZFN1YnRyYWN0aW9uKGludDMyX3QgYSwgaW50MzJfdCBiKQo=