156266 2016-04-05 17:21:44 -0700 Implement operator== and operator!= for Optional<> 2016-04-06 12:04:00 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 156274 1 simon.fraser simon.fraser andersca benjamin cdumez cmarcelo commit-queue mmaxfield sam simon.fraser oldest_to_newest 1181083 0 simon.fraser 2016-04-05 17:21:44 -0700 Implement operator== and operator!= for Optional<> 1181084 1 275726 simon.fraser 2016-04-05 17:22:48 -0700 Created attachment 275726 Patch 1181085 2 commit-queue 2016-04-05 17:23:33 -0700 Attachment 275726 did not pass style-queue: ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:115: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:116: Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:117: Consider using EXPECT_NE instead of EXPECT_FALSE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:118: Consider using EXPECT_NE instead of EXPECT_FALSE(a == b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:140: Consider using EXPECT_EQ instead of EXPECT_FALSE(a != b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:141: Consider using EXPECT_NE instead of EXPECT_TRUE(a != b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:142: Consider using EXPECT_EQ instead of EXPECT_FALSE(a != b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:143: Consider using EXPECT_NE instead of EXPECT_TRUE(a != b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:145: Consider using EXPECT_NE instead of EXPECT_TRUE(a != b) [readability/check] [2] ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:146: Consider using EXPECT_NE instead of EXPECT_TRUE(a != b) [readability/check] [2] Total errors found: 10 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style. 1181088 3 275726 mmaxfield 2016-04-05 17:28:33 -0700 Comment on attachment 275726 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275726&action=review I think I should wait for Anders to give the r+, but LGTM. > Source/WTF/wtf/Optional.h:202 > + return static_cast<bool>(lhs) == static_cast<bool>(rhs) && (!static_cast<bool>(lhs) || lhs.value() == rhs.value()); Wouldn't an if statement be cleaner? Are these taken directly from C++ docs? > Source/WTF/wtf/Optional.h:256 > + return !(value == opt); Perhaps it would be useful to make one with two template types, and static_cast one to the other, to try to compare things which aren't the same type but are convertible. 1181092 4 simon.fraser 2016-04-05 17:31:30 -0700 (In reply to comment #2) > Attachment 275726 [details] did not pass style-queue: > > > ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:115: Consider using > EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] Using these didn't compile.(In reply to comment #3) > Comment on attachment 275726 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275726&action=review > > I think I should wait for Anders to give the r+, but LGTM. > > > Source/WTF/wtf/Optional.h:202 > > + return static_cast<bool>(lhs) == static_cast<bool>(rhs) && (!static_cast<bool>(lhs) || lhs.value() == rhs.value()); > > Wouldn't an if statement be cleaner? But this is branchless. > > Are these taken directly from C++ docs? Some are. 1181313 5 andersca 2016-04-06 11:35:18 -0700 (In reply to comment #4) > (In reply to comment #2) > > Attachment 275726 [details] did not pass style-queue: > > > > > > ERROR: Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:115: Consider using > > EXPECT_EQ instead of EXPECT_TRUE(a == b) [readability/check] [2] > > Using these didn't compile.(In reply to comment #3) > > > Comment on attachment 275726 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=275726&action=review > > > > I think I should wait for Anders to give the r+, but LGTM. > > > > > Source/WTF/wtf/Optional.h:202 > > > + return static_cast<bool>(lhs) == static_cast<bool>(rhs) && (!static_cast<bool>(lhs) || lhs.value() == rhs.value()); > > > > Wouldn't an if statement be cleaner? > > But this is branchless. Pretty sure it isn't branchless. && and || always form branches. Besides, the compiler will be able to get rid of unnecessary branches - even if you use if statements. > > > > Are these taken directly from C++ docs? > > Some are. 1181349 6 simon.fraser 2016-04-06 12:04:00 -0700 http://trac.webkit.org/changeset/199107 275726 2016-04-05 17:22:48 -0700 2016-04-06 11:36:00 -0700 Patch bug-156266-20160405172249.patch text/plain 4921 simon.fraser U3VidmVyc2lvbiBSZXZpc2lvbjogMTk5MDY0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV1RGL0NoYW5n ZUxvZyBiL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCmluZGV4IGM3NzNjZTg4MjYwMDg1MzA3YzY4YTJm NzUwYWM1NDA5YjBhYjE5ZmYuLjRkODlmMDNhZTllZTgwOWIxNzU1NmYwODQzZWQ2YTRlM2JkMDhk ZmEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XVEYvQ2hh bmdlTG9nCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTYtMDQtMDUgIFNpbW9uIEZyYXNlciAgPHNpbW9u LmZyYXNlckBhcHBsZS5jb20+CisKKyAgICAgICAgSW1wbGVtZW50IG9wZXJhdG9yPT0gYW5kIG9w ZXJhdG9yIT0gZm9yIE9wdGlvbmFsPD4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTE1NjI2NgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorICAgICAgICAKKyAgICAgICAgSW1wbGVtZW50IG5vbi1tZW1iZXIgb3BlcmF0b3I9PSBh bmQgb3BlcmF0b3IhPSB2YXJpYW50cyBmb3IgT3B0aW9uYWw8Pi4KKworICAgICAgICAqIHd0Zi9P cHRpb25hbC5oOgorICAgICAgICAoV1RGOjpvcGVyYXRvcj09KToKKyAgICAgICAgKFdURjo6b3Bl cmF0b3IhPSk6CisKIDIwMTYtMDQtMDUgIEFudG9pbmUgUXVpbnQgIDxncmFvdXRzQGFwcGxlLmNv bT4KIAogICAgICAgICBbV2ViR0wyXSBBbGxvdyBlbmFibGluZyBXZWJHTDIgd2l0aCBhIHJ1bnRp bWUgZmxhZwpkaWZmIC0tZ2l0IGEvU291cmNlL1dURi93dGYvT3B0aW9uYWwuaCBiL1NvdXJjZS9X VEYvd3RmL09wdGlvbmFsLmgKaW5kZXggZmEwZWRmN2RmMzI4MDIyODNhMzU3MzgyYWUxYzY2YWFj NDJhNmZmZi4uNGNiNTNiZjU3ZjgyOWU0NGRhOWZiZDQ5ZDZmZDljOGM5OGM5NDRkNCAxMDA2NDQK LS0tIGEvU291cmNlL1dURi93dGYvT3B0aW9uYWwuaAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9PcHRp b25hbC5oCkBAIC0xOTcsNiArMTk3LDY2IEBAIHByaXZhdGU6CiB9OwogCiB0ZW1wbGF0ZTx0eXBl bmFtZSBUPgorY29uc3RleHByIGJvb2wgb3BlcmF0b3I9PShjb25zdCBPcHRpb25hbDxUPiYgbGhz LCBjb25zdCBPcHRpb25hbDxUPiYgcmhzKQoreworICAgIHJldHVybiBzdGF0aWNfY2FzdDxib29s PihsaHMpID09IHN0YXRpY19jYXN0PGJvb2w+KHJocykgJiYgKCFzdGF0aWNfY2FzdDxib29sPihs aHMpIHx8IGxocy52YWx1ZSgpID09IHJocy52YWx1ZSgpKTsKK30KKwordGVtcGxhdGU8dHlwZW5h bWUgVD4KK2NvbnN0ZXhwciBib29sIG9wZXJhdG9yIT0oY29uc3QgT3B0aW9uYWw8VD4mIGxocywg Y29uc3QgT3B0aW9uYWw8VD4mIHJocykKK3sKKyAgICByZXR1cm4gIShsaHMgPT0gcmhzKTsKK30K KwordGVtcGxhdGU8dHlwZW5hbWUgVD4KK2NvbnN0ZXhwciBib29sIG9wZXJhdG9yPT0oY29uc3Qg T3B0aW9uYWw8VD4mIG9wdCwgTnVsbG9wdFRhZykKK3sKKyAgICByZXR1cm4gIW9wdDsKK30KKwor dGVtcGxhdGU8dHlwZW5hbWUgVD4KK2NvbnN0ZXhwciBib29sIG9wZXJhdG9yIT0oY29uc3QgT3B0 aW9uYWw8VD4mIG9wdCwgTnVsbG9wdFRhZykKK3sKKyAgICByZXR1cm4gc3RhdGljX2Nhc3Q8Ym9v bD4ob3B0KTsKK30KKwordGVtcGxhdGU8dHlwZW5hbWUgVD4KK2NvbnN0ZXhwciBib29sIG9wZXJh dG9yPT0oTnVsbG9wdFRhZywgY29uc3QgT3B0aW9uYWw8VD4mIG9wdCkKK3sKKyAgICByZXR1cm4g IW9wdDsKK30KKwordGVtcGxhdGU8dHlwZW5hbWUgVD4KK2NvbnN0ZXhwciBib29sIG9wZXJhdG9y IT0oTnVsbG9wdFRhZywgY29uc3QgT3B0aW9uYWw8VD4mIG9wdCkKK3sKKyAgICByZXR1cm4gc3Rh dGljX2Nhc3Q8Ym9vbD4ob3B0KTsKK30KKwordGVtcGxhdGU8dHlwZW5hbWUgVD4KK2NvbnN0ZXhw ciBib29sIG9wZXJhdG9yPT0oY29uc3QgT3B0aW9uYWw8VD4mIG9wdCwgY29uc3QgVCYgdmFsdWUp Cit7CisgICAgcmV0dXJuIG9wdCAmJiBvcHQudmFsdWUoKSA9PSB2YWx1ZTsKK30KKwordGVtcGxh dGU8dHlwZW5hbWUgVD4KK2NvbnN0ZXhwciBib29sIG9wZXJhdG9yIT0oY29uc3QgT3B0aW9uYWw8 VD4mIG9wdCwgY29uc3QgVCYgdmFsdWUpCit7CisgICAgcmV0dXJuICEob3B0ID09IHZhbHVlKTsK K30KKwordGVtcGxhdGU8dHlwZW5hbWUgVD4KK2NvbnN0ZXhwciBib29sIG9wZXJhdG9yPT0oY29u c3QgVCYgdmFsdWUsIGNvbnN0IE9wdGlvbmFsPFQ+JiBvcHQpCit7CisgICAgcmV0dXJuIG9wdCAm JiBvcHQudmFsdWUoKSA9PSB2YWx1ZTsKK30KKwordGVtcGxhdGU8dHlwZW5hbWUgVD4KK2NvbnN0 ZXhwciBib29sIG9wZXJhdG9yIT0oY29uc3QgVCYgdmFsdWUsIGNvbnN0IE9wdGlvbmFsPFQ+JiBv cHQpCit7CisgICAgcmV0dXJuICEodmFsdWUgPT0gb3B0KTsKK30KKwordGVtcGxhdGU8dHlwZW5h bWUgVD4KIE9wdGlvbmFsPHR5cGVuYW1lIHN0ZDo6ZGVjYXk8VD46OnR5cGU+CiBtYWtlT3B0aW9u YWwoVCYmIHZhbHVlKQogewpkaWZmIC0tZ2l0IGEvVG9vbHMvQ2hhbmdlTG9nIGIvVG9vbHMvQ2hh bmdlTG9nCmluZGV4IDJjMDQxN2JlNzUxZWJmMzhkZTFhNWMwYTUyNjUwZDA3MmQ2NjU5OGMuLjQ5 NzU5MDg4NjAwMzcxNWI1ZWQ2ODFiMzFkNjQ2MzE1ODI5MTI3MjIgMTAwNjQ0Ci0tLSBhL1Rvb2xz L0NoYW5nZUxvZworKysgYi9Ub29scy9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxMyBAQAorMjAxNi0w NC0wNSAgU2ltb24gRnJhc2VyICA8c2ltb24uZnJhc2VyQGFwcGxlLmNvbT4KKworICAgICAgICBJ bXBsZW1lbnQgb3BlcmF0b3I9PSBhbmQgb3BlcmF0b3IhPSBmb3IgT3B0aW9uYWw8PgorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTU2MjY2CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBUZXN0V2ViS2l0QVBJ L1Rlc3RzL1dURi9PcHRpb25hbC5jcHA6CisgICAgICAgIChUZXN0V2ViS2l0QVBJOjpURVNUKToK KwogMjAxNi0wNC0wNSAgQW50b2luZSBRdWludCAgPGdyYW91dHNAYXBwbGUuY29tPgogCiAgICAg ICAgIFtXZWJHTDJdIEFsbG93IGVuYWJsaW5nIFdlYkdMMiB3aXRoIGEgcnVudGltZSBmbGFnCmRp ZmYgLS1naXQgYS9Ub29scy9UZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9PcHRpb25hbC5jcHAgYi9U b29scy9UZXN0V2ViS2l0QVBJL1Rlc3RzL1dURi9PcHRpb25hbC5jcHAKaW5kZXggMDVmNDEzYjk3 YmIwYzcxYjU1NzQ1OTdlMDJiM2RhMGQxMDJlZjZhZC4uYTA3MTVmZDYyZmU3NmMxMDJjNDQzYTFj Nzc0NmY2ZDRiODhjNzRiYiAxMDA2NDQKLS0tIGEvVG9vbHMvVGVzdFdlYktpdEFQSS9UZXN0cy9X VEYvT3B0aW9uYWwuY3BwCisrKyBiL1Rvb2xzL1Rlc3RXZWJLaXRBUEkvVGVzdHMvV1RGL09wdGlv bmFsLmNwcApAQCAtOTMsNCArOTMsNTggQEAgVEVTVChXVEZfT3B0aW9uYWwsIENhbGxiYWNrKQog ICAgIEVYUEVDVF9FUShyZXN1bHQsIDI1MCk7CiB9CiAKK1RFU1QoV1RGX09wdGlvbmFsLCBFcXVh bGl0eSkKK3sKKyAgICBPcHRpb25hbDxpbnQ+IHVuZW5nYWdlZDE7CisgICAgT3B0aW9uYWw8aW50 PiB1bmVuZ2FnZWQyOworCisgICAgT3B0aW9uYWw8aW50PiBlbmdhZ2VkMSB7IDEgfTsKKyAgICBP cHRpb25hbDxpbnQ+IGVuZ2FnZWQyIHsgMiB9OworICAgIE9wdGlvbmFsPGludD4gZW5nYWdlZHgy IHsgMiB9OworCisgICAgRVhQRUNUX1RSVUUodW5lbmdhZ2VkMSA9PSB1bmVuZ2FnZWQyKTsKKyAg ICBFWFBFQ1RfRkFMU0UoZW5nYWdlZDEgPT0gZW5nYWdlZDIpOworICAgIEVYUEVDVF9GQUxTRShl bmdhZ2VkMSA9PSB1bmVuZ2FnZWQxKTsKKyAgICBFWFBFQ1RfVFJVRShlbmdhZ2VkMiA9PSBlbmdh Z2VkeDIpOworCisgICAgRVhQRUNUX1RSVUUodW5lbmdhZ2VkMSA9PSBOdWxsb3B0KTsKKyAgICBF WFBFQ1RfRkFMU0UoZW5nYWdlZDEgPT0gTnVsbG9wdCk7CisgICAgRVhQRUNUX1RSVUUoTnVsbG9w dCA9PSB1bmVuZ2FnZWQxKTsKKyAgICBFWFBFQ1RfRkFMU0UoTnVsbG9wdCA9PSBlbmdhZ2VkMSk7 CisKKyAgICBFWFBFQ1RfVFJVRShlbmdhZ2VkMSA9PSAxKTsKKyAgICBFWFBFQ1RfVFJVRSgxID09 IGVuZ2FnZWQxKTsKKyAgICBFWFBFQ1RfRkFMU0UodW5lbmdhZ2VkMSA9PSAxKTsKKyAgICBFWFBF Q1RfRkFMU0UoMSA9PSB1bmVuZ2FnZWQxKTsKK30KKworVEVTVChXVEZfT3B0aW9uYWwsIEluZXF1 YWxpdHkpCit7CisgICAgT3B0aW9uYWw8aW50PiB1bmVuZ2FnZWQxOworICAgIE9wdGlvbmFsPGlu dD4gdW5lbmdhZ2VkMjsKKworICAgIE9wdGlvbmFsPGludD4gZW5nYWdlZDEgeyAxIH07CisgICAg T3B0aW9uYWw8aW50PiBlbmdhZ2VkMiB7IDIgfTsKKyAgICBPcHRpb25hbDxpbnQ+IGVuZ2FnZWR4 MiB7IDIgfTsKKworICAgIEVYUEVDVF9GQUxTRSh1bmVuZ2FnZWQxICE9IHVuZW5nYWdlZDIpOwor ICAgIEVYUEVDVF9UUlVFKGVuZ2FnZWQxICE9IGVuZ2FnZWQyKTsKKyAgICBFWFBFQ1RfVFJVRShl bmdhZ2VkMSAhPSB1bmVuZ2FnZWQxKTsKKyAgICBFWFBFQ1RfRkFMU0UoZW5nYWdlZDIgIT0gZW5n YWdlZHgyKTsKKworICAgIEVYUEVDVF9GQUxTRSh1bmVuZ2FnZWQxICE9IE51bGxvcHQpOworICAg IEVYUEVDVF9UUlVFKGVuZ2FnZWQxICE9IE51bGxvcHQpOworICAgIEVYUEVDVF9GQUxTRShOdWxs b3B0ICE9IHVuZW5nYWdlZDEpOworICAgIEVYUEVDVF9UUlVFKE51bGxvcHQgIT0gZW5nYWdlZDEp OworCisgICAgRVhQRUNUX0ZBTFNFKGVuZ2FnZWQxICE9IDEpOworICAgIEVYUEVDVF9UUlVFKGVu Z2FnZWQxICE9IDIpOworICAgIEVYUEVDVF9GQUxTRSgxICE9IGVuZ2FnZWQxKTsKKyAgICBFWFBF Q1RfVFJVRSgyICE9IGVuZ2FnZWQxKTsKKworICAgIEVYUEVDVF9UUlVFKHVuZW5nYWdlZDEgIT0g MSk7CisgICAgRVhQRUNUX1RSVUUoMSAhPSB1bmVuZ2FnZWQxKTsKK30KKworCiB9IC8vIG5hbWVz cGFjZSBUZXN0V2ViS2l0QVBJCg==