Bug 164526 – Implement WTF::Expected

• Component: Web Template Framework
• Status: RESOLVED FIXED

[JF Bastien]

As described in http://wg21.link/p0323r1

std::expected isn't in C++17, and may be in C++20. It's a nice complement to std::any / std::optional because it's a type-tagged union which has a single expected result but could also contain an error.

This would be useful in the WebAssembly parser, for example.

Using this implementation will allow us to provide feedback to the standards committee and guide std::expected's design before it gets standardized. I've already sent a bunch of feedback to the author based on my experience implementing this.

[JF Bastien]

Yusuke points out:
  WTF::Expect is something like Either<V, E>, right? I think you can propose changing ExceptionOr thingy to your WTF::Expect. Binding refactoring with ExceptionOr is ongoing right now!
  template<typename T> using ExceptionOr<T> = WTF::Expect<T, SomeExceptionClass>;

I'd searched around WTF but failed to see this. I'll upload what I currently have, and then see how compatible these are. Either way (ha!) we may want to feedback these differences to the committee.

[Yusuke Suzuki]

(In reply to comment #1)
> Yusuke points out:
>   WTF::Expect is something like Either<V, E>, right? I think you can propose
> changing ExceptionOr thingy to your WTF::Expect. Binding refactoring with
> ExceptionOr is ongoing right now!
>   template<typename T> using ExceptionOr<T> = WTF::Expect<T,
> SomeExceptionClass>;
> 
> I'd searched around WTF but failed to see this. I'll upload what I currently
> have, and then see how compatible these are. Either way (ha!) we may want to
> feedback these differences to the committee.

You can find it in WebCore/dom/ExceptionOr.h.
It seems like std::expected is nice for ExceptionOr. It has what we want, like, std::expected<void, error> specialization!

[JF Bastien]

Created attachment 294206 [details]
patch

Proposed patch.

It is currently very close to what's proposed in http://wg21.link/p0323r1 except:

 - Follows WebKit naming convention (except where required, e.g. for std::hash).
 - Has a few check-webkit-style fails because I think check-webkit-style is wrong (I've read all the warnings and don't think they're relevant, let me know if that's not the case).
 - A few APIs are missing. I've left them as commented-out lines for now. They don't seem critical to getting this off the ground.
 - A few new APIs are available, in every case I've documented this and sent feedback to the author.
 - A few things are different e.g. std::nullopt_t is the default (the current proposal is missing a default here).
 - No exceptions. It instead calls RELEASE_ASSERT_NOT_REACHED. Note that not all accessors do this! It's what the current proposal does. I could have debug asserts for these?
 - No [[nodiscard]] for now. I think it would be nice to add.

I've been running the tests using the cmake build:
  (cd current/ && ninja TestWTF && ./bin/TestWTF --gtest_filter="*xpected*" ; echo $?)

Note that this patch is very careful about constexpr+literal types, as well as supporting non-literal types. If something is weird that may be why.

If you like the current patch I can look at what we could do w.r.t. WTF::Either and WTF::ExceptionOr. How much can we replace, and how much of the proposal should we change to give us more of what we want? Let me know, I've been providing feedback to the committee.

[WebKit Commit Bot]

Attachment 294206 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:339:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:340:  Consider using EXPECT_NE instead of EXPECT_TRUE(a != b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:340:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:341:  Consider using EXPECT_LT instead of EXPECT_TRUE(a < b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:342:  Consider using EXPECT_GT instead of EXPECT_TRUE(a > b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:343:  Consider using EXPECT_LE instead of EXPECT_TRUE(a <= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:344:  Consider using EXPECT_GE instead of EXPECT_TRUE(a >= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:345:  Consider using EXPECT_LE instead of EXPECT_TRUE(a <= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:346:  Consider using EXPECT_GE instead of EXPECT_TRUE(a >= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:348:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:348:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:349:  Consider using EXPECT_EQ instead of EXPECT_FALSE(a != b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:350:  Consider using EXPECT_GE instead of EXPECT_FALSE(a < b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:351:  Consider using EXPECT_LE instead of EXPECT_FALSE(a > b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:352:  Consider using EXPECT_GE instead of EXPECT_FALSE(a < b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:353:  Consider using EXPECT_LT instead of EXPECT_FALSE(a >= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:355:  Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:356:  Consider using EXPECT_NE instead of EXPECT_TRUE(a != b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:357:  Consider using EXPECT_LT instead of EXPECT_TRUE(a < b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:358:  Consider using EXPECT_GT instead of EXPECT_TRUE(a > b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:359:  Consider using EXPECT_LE instead of EXPECT_TRUE(a <= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:360:  Consider using EXPECT_GE instead of EXPECT_TRUE(a >= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:361:  Consider using EXPECT_LE instead of EXPECT_TRUE(a <= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:362:  Consider using EXPECT_GE instead of EXPECT_TRUE(a >= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:364:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:365:  Consider using EXPECT_EQ instead of EXPECT_FALSE(a != b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:366:  Consider using EXPECT_GE instead of EXPECT_FALSE(a < b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:367:  Consider using EXPECT_LE instead of EXPECT_FALSE(a > b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:368:  Consider using EXPECT_GT instead of EXPECT_FALSE(a <= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:369:  Consider using EXPECT_LT instead of EXPECT_FALSE(a >= b)  [readability/check] [2]
ERROR: Source/WTF/wtf/Expected.h:47:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:69:  Do not use unnamed namespaces in header files.  See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces for more information.  [build/namespaces] [4]
ERROR: Source/WTF/wtf/Expected.h:170:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:172:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:216:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:253:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:264:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:265:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:267:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:273:  Use 'using namespace std;' instead of 'using std::swap;'.  [build/using_std] [4]
ERROR: Source/WTF/wtf/Expected.h:277:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:278:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:282:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:283:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:294:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:295:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:308:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:335:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:337:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:342:  Use 'using namespace std;' instead of 'using std::swap;'.  [build/using_std] [4]
ERROR: Source/WTF/wtf/Expected.h:346:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:350:  Use 'WTFMove()' instead of 'std::move()'.  [runtime/wtf_move] [4]
ERROR: Source/WTF/wtf/Expected.h:421:  result_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/Expected.h:427:  result_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 54 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yusuke Suzuki]

Comment on attachment 294206 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294206&action=review

r=me. I think the name is OK. Current WTF::Expected is slightly different from std::experimental::expected proposal. So leaving it as WTF::Expected is reasonable to me.

> Source/WTF/wtf/Expected.h:47
> +    constexpr explicit UnexpectedType(E&& e) : val(std::move(e)) { }

Since this implementation is written for WTF tree, we can use WTFMove instead of std::move here.

> Source/WTF/wtf/Expected.h:231
> +>::type;

Can we use WTF::Variant as a base of this WTF::Expected implementation? If we can't, why?

> Source/WTF/wtf/Expected.h:433
> +using WTF::Nullopt;

This line is not necessary. It is done by Optional.h.

> Tools/ChangeLog:23
> [phone]  JF Bastien  <[e-mail]>
> +
> +        Implement WTF::Expected
> +        https://bugs.webkit.org/show_bug.cgi?id=164526
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * TestWebKitAPI/CMakeLists.txt:
> +        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
> +        * TestWebKitAPI/Tests/WTF/Expected.cpp: Added.
> +        (TestWebKitAPI::TEST):

ChangeLog is duplicated.

> Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:290
> +    EXPECT_TRUE(Ex(42) == Ex(42));

Let's use EXPECT_EQ and address the warnings reported by webkit-check-style :)

[Darin Adler]

I like the idea of replacing ExceptionOr with std::experimental::expected. The concepts are the same.

The idioms and naming are quite different; because of that I don’t think we should use template typedef. It’s probably a moderately big project to modify all the recently-minted ExceptionOr call sites to use the new syntax so we probably would want to do in stages. I presume we would add functions to ExceptionOr that match the names and semantics of the std::experimental::expected functions, then migrate to the new functions, and then move to template typedef, then finally consider actually moving to std::experimental::expected.

The proposal does include "expected void", which is good, since ExceptionOr<void> seems to be working well.

I think most of the learning about this will come from trying to use it at all the call sites and seeing if the code gets more readable or more confusing.

The proposal throws exceptions when functions are called incorrectly. That won’t work for us; we want it to assert or abort in those cases.

I’m a little sad to learn about this only after I converted all our code to ExceptionOr; could have switched directly to this instead of having to revisit everything.

[JF Bastien]

Created attachment 294336 [details]
patch

I've addressed the following comments:

 - Remove Nullopt (which I was already getting from Optional, used to define my own).
 - Remove duplicate ChangeLog entry.
 - Use WTFMove, and remove erroneous copy-paste const on move ctor.
 - Implement operator<<(stream) to use EXPECT_* and silence warnings. It's purposefully not in the proposal, but having it in the test does make things simpler. check-webkit-style will still be sad when I use EXPECT_FALSE, please ignore it, it's silly.

I still have to try out WTF::Variant. I'll do this soon.

[JF Bastien]

(In reply to comment #6)
> I like the idea of replacing ExceptionOr with std::experimental::expected.
> The concepts are the same.
> 
> The idioms and naming are quite different; because of that I don’t think we
> should use template typedef. It’s probably a moderately big project to
> modify all the recently-minted ExceptionOr call sites to use the new syntax
> so we probably would want to do in stages. I presume we would add functions
> to ExceptionOr that match the names and semantics of the
> std::experimental::expected functions, then migrate to the new functions,
> and then move to template typedef, then finally consider actually moving to
> std::experimental::expected.

That sounds fine, I'm happy to do this asynchronously.


> The proposal does include "expected void", which is good, since
> ExceptionOr<void> seems to be working well.
> 
> I think most of the learning about this will come from trying to use it at
> all the call sites and seeing if the code gets more readable or more
> confusing.

Right, I think the window is *way* open to make sure the C++20 version is nice.

 
> The proposal throws exceptions when functions are called incorrectly. That
> won’t work for us; we want it to assert or abort in those cases.

My implementation aborts instead of throwing.


> I’m a little sad to learn about this only after I converted all our code to
> ExceptionOr; could have switched directly to this instead of having to
> revisit everything.

Sorry? :-)

[JF Bastien]

I've been playing with WTF::Variant as the storage, and I don't think it's a great fit:

 - Variant is designed to hold an unbounded number of values, so the index has to be implemented in a clever manner to fit in the smallest storage (potentially within the end-pad of what's held). I can just use a bit (I'm using a byte right now because it's easier to read and realistically is OK).
 - There's gymnastics around "valueless by exception" which Expected just avoids.
 - Expected<T, T> is sensible, whereas Variant<T, T> is weird. It's allowed but is weird.
 - Expected<void, E> seems easier here, but I may have implementor's bias.

Further, I want std::expected to become more opinionated when something unexpected happens: I think destroying a std::expected which contains an error, without having retrieved the error first, should throw (abort for us). That's easy to do in my current implementation ("bool has" can be made into a bitmask), but I need extra storage if I use Variant (I can't hijack its index for that purpose). I've already proposed this to the committee, but need to try it out at scale to see how useful / impactful it is.


What do you think Yusuke?


I think the next step is for me to clean up the Windows and GTK builds. :-)

[Yusuke Suzuki]

OK, I think it is reasonable.
In particular, Variant<T, T> v.s. Expected<T, T> is a good reason to use WTF::Expected's own storage here.

(In reply to comment #9)
> I've been playing with WTF::Variant as the storage, and I don't think it's a
> great fit:
> 
>  - Variant is designed to hold an unbounded number of values, so the index
> has to be implemented in a clever manner to fit in the smallest storage
> (potentially within the end-pad of what's held). I can just use a bit (I'm
> using a byte right now because it's easier to read and realistically is OK).

Yup.

>  - There's gymnastics around "valueless by exception" which Expected just
> avoids.

Is just hiding these methods behind the WTF::Expected enough?

>  - Expected<T, T> is sensible, whereas Variant<T, T> is weird. It's allowed
> but is weird.

That's good point!! Yeah, it should be allowed in WTF::Expected. But it is weird in WTF::Variant.

>  - Expected<void, E> seems easier here, but I may have implementor's bias.

In that case, we can just use the WTF::Optional for the storage of Expected<void, E> case in the future. (I need to land WTF::Optional patch, working on that.)

> 
> Further, I want std::expected to become more opinionated when something
> unexpected happens: I think destroying a std::expected which contains an
> error, without having retrieved the error first, should throw (abort for
> us). That's easy to do in my current implementation ("bool has" can be made
> into a bitmask), but I need extra storage if I use Variant (I can't hijack
> its index for that purpose). I've already proposed this to the committee,
> but need to try it out at scale to see how useful / impactful it is.
> 

Interesting. Yeah, in that case, managing the own storage can save some bytes.

> 
> What do you think Yusuke?
> 
> 
> I think the next step is for me to clean up the Windows and GTK builds. :-)

GTK build seems related to move accessors & reference accessors. They are not supported in the old gcc.
Due to some distributions, we need to stick on old GCC, that's so sad :(.
https://lists.webkit.org/pipermail/webkit-dev/2016-September/028438.html

[JF Bastien]

Created attachment 294473 [details]
patch

Address some of the GTK bot's complaints:
 - Avoid anonymous namespace.
 - Add RELAXED_CONSTEXPR because GTK's version of GCC (4.9 or earlier?) doesn't support C++14 relaxed constant expressions.

I don't think this fixes all of GTK's sads, but it'll fix some. I'll have to look at whatever falls out of GTK next, and Windows.

[WebKit Commit Bot]

Attachment 294473 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:374:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:374:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:375:  Consider using EXPECT_EQ instead of EXPECT_FALSE(a != b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:376:  Consider using EXPECT_GE instead of EXPECT_FALSE(a < b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:377:  Consider using EXPECT_LE instead of EXPECT_FALSE(a > b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:378:  Consider using EXPECT_GE instead of EXPECT_FALSE(a < b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:379:  Consider using EXPECT_LT instead of EXPECT_FALSE(a >= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:390:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:391:  Consider using EXPECT_EQ instead of EXPECT_FALSE(a != b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:392:  Consider using EXPECT_GE instead of EXPECT_FALSE(a < b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:393:  Consider using EXPECT_LE instead of EXPECT_FALSE(a > b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:394:  Consider using EXPECT_GT instead of EXPECT_FALSE(a <= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:395:  Consider using EXPECT_LT instead of EXPECT_FALSE(a >= b)  [readability/check] [2]
ERROR: Source/WTF/wtf/Expected.h:230:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Expected.h:274:  Use 'using namespace std;' instead of 'using std::swap;'.  [build/using_std] [4]
ERROR: Source/WTF/wtf/Expected.h:343:  Use 'using namespace std;' instead of 'using std::swap;'.  [build/using_std] [4]
ERROR: Source/WTF/wtf/Expected.h:422:  result_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/Expected.h:428:  result_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 18 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 294570 [details]
patch

GTK was happy in the last patch, and MSVC got happier. Try to resolve the last MSVC issue (I think it doesn't like explicitly destroying trivially-destructible types).

[WebKit Commit Bot]

Attachment 294570 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:374:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:374:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:375:  Consider using EXPECT_EQ instead of EXPECT_FALSE(a != b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:376:  Consider using EXPECT_GE instead of EXPECT_FALSE(a < b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:377:  Consider using EXPECT_LE instead of EXPECT_FALSE(a > b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:378:  Consider using EXPECT_GE instead of EXPECT_FALSE(a < b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:379:  Consider using EXPECT_LT instead of EXPECT_FALSE(a >= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:390:  Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:391:  Consider using EXPECT_EQ instead of EXPECT_FALSE(a != b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:392:  Consider using EXPECT_GE instead of EXPECT_FALSE(a < b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:393:  Consider using EXPECT_LE instead of EXPECT_FALSE(a > b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:394:  Consider using EXPECT_GT instead of EXPECT_FALSE(a <= b)  [readability/check] [2]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:395:  Consider using EXPECT_LT instead of EXPECT_FALSE(a >= b)  [readability/check] [2]
ERROR: Source/WTF/wtf/Expected.h:233:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WTF/wtf/Expected.h:277:  Use 'using namespace std;' instead of 'using std::swap;'.  [build/using_std] [4]
ERROR: Source/WTF/wtf/Expected.h:346:  Use 'using namespace std;' instead of 'using std::swap;'.  [build/using_std] [4]
ERROR: Source/WTF/wtf/Expected.h:425:  result_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/WTF/wtf/Expected.h:431:  result_type is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 18 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Comment on attachment 294570 [details]
patch

Wooh! GTK and MSVC pass :-)
Good to go?

[Yusuke Suzuki]

Comment on attachment 294570 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=294570&action=review

r=me with comments. Nice.

> Source/WTF/ChangeLog:4
> +        Implement WTF::Expected
> +

This is not necessary

> Source/WTF/ChangeLog:18
> +        std::expected isn't in C++17, and may be in C++20. It's a nice
> +        complement to std::any / std::optional because it's a type-tagged
> +        union which has a single expected result but could also contain an
> +        error.
> +
> +        This would be useful in the WebAssembly parser, for example.
> +
> +        Using this implementation will allow us to provide feedback to the
> +        standards committee and guide std::expected's design before it
> +        gets standardized. I've already sent a bunch of feedback to the
> +        author based on my experience implementing this.
> +
> +        This could supplement WTF::Either and WTF::ExceptionOr.
> +

These lines should be L23~.

> Tools/ChangeLog:22
> +        Implement WTF::Expected
> +
> +        std::expected isn't in C++17, and may be in C++20. It's a nice
> +        complement to std::any / std::optional because it's a type-tagged
> +        union which has a single expected result but could also contain an
> +        error.
> +
> +        This would be useful in the WebAssembly parser, for example.
> +
> +        Using this implementation will allow us to provide feedback to the
> +        standards committee and guide std::expected's design before it
> +        gets standardized. I've already sent a bunch of feedback to the
> +        author based on my experience implementing this.
> +
> +        This could supplement WTF::Either and WTF::ExceptionOr.
> +
> +        Implement WTF::Expected
> +        https://bugs.webkit.org/show_bug.cgi?id=164526
> +
> +        Reviewed by NOBODY (OOPS!).

Ditto.

> Tools/TestWebKitAPI/Tests/WTF/Expected.cpp:88
> +}

Can you add a test to ensure the correct destructor is called?

[JF Bastien]

Created attachment 294670 [details]
patch

Address comments:

 - Fix ChangeLog.
 - Add destructor test.

Also fix CMake build which was broken by:
  https://bugs.webkit.org/show_bug.cgi?id=164595

[WebKit Commit Bot]

Comment on attachment 294670 [details]
patch

Clearing flags on attachment: 294670

Committed r208670: <http://trac.webkit.org/changeset/208670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.