Bug 156266 – Implement operator== and operator!= for Optional<>

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Simon Fraser (smfr)]

Implement operator== and operator!= for Optional<>

[Simon Fraser (smfr)]

Created attachment 275726 [details]
Patch

[WebKit Commit Bot]

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]
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.

[Myles C. Maxfield]

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?

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.

[Simon Fraser (smfr)]

(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.

[Anders Carlsson]

(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.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/199107