.
Created attachment 429502 [details] Patch
Comment on attachment 429502 [details] Patch Please add a test. (TestWTF)
Created attachment 429503 [details] Patch
Comment on attachment 429503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429503&action=review > Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:253 > + Optional<int&> optional { x }; In all the places where you want to use an optional reference, you should just be using a pointer instead. You shouldn't have hit this problem in the first place.
Using `const T*` works for me. Should the Optional<T&> specialization be removed? There are a few places in the codebase that use Optional<T&>, specifically Optional<JSObject&>. Yusuke, looks like you added the Optional<T&> specialization. Do you think it's really needed, or should we just be using JSObject* in these cases? I thought perhaps this was for completeness when compared to std::optional, but https://en.cppreference.com/w/cpp/utility/optional says: There are no optional references; a program is ill-formed if it instantiates an optional with a reference type.
Comment on attachment 429503 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=429503&action=review >> Tools/TestWebKitAPI/Tests/WTF/Optional.cpp:253 >> + Optional<int&> optional { x }; > > In all the places where you want to use an optional reference, you should just be using a pointer instead. You shouldn't have hit this problem in the first place. I agree with this comment. As a style choice we encourage continuing to use pointers for optional references.
(In reply to Cameron McCormack (:heycam) from comment #5) > Using `const T*` works for me. Should the Optional<T&> specialization be > removed? > > There are a few places in the codebase that use Optional<T&>, specifically > Optional<JSObject&>. Yusuke, looks like you added the Optional<T&> > specialization. Do you think it's really needed, or should we just be using > JSObject* in these cases? > > I thought perhaps this was for completeness when compared to std::optional, > but https://en.cppreference.com/w/cpp/utility/optional says: > > There are no optional references; a program is ill-formed if it > instantiates an > optional with a reference type. Indeed, it appears std::optional doesn't support this: https://www.fluentcpp.com/2018/10/05/pros-cons-optional-references/ Maybe we should just get rid of the functionality altogether in WTF::Optional? Let's see what Yusuke has to say since he added it. I do agree that a T* makes more sense than an Optional<T&>.
(In reply to Cameron McCormack (:heycam) from comment #5) > I thought perhaps this was for completeness when compared to std::optional, > but https://en.cppreference.com/w/cpp/utility/optional says: > > There are no optional references; a program is ill-formed if it > instantiates an > optional with a reference type. Very interesting! I would like our WTF::Optional to match std::optional and I intend to switch us off of it. You can see a brief debate in bug 211674 where Chris Dumez suggests that our different semantic of guaranteeing a WTF::Optional is set to WTF::nullopt when moving from it is a reason to stay with WTF::Optional. I’d like to resolve that issue and dump WTF::Optional entirely as soon as we can. In the mean time, I suggest we minimize differences from std::optional, which points to removing our support for optional references.
(In reply to Darin Adler from comment #8) > (In reply to Cameron McCormack (:heycam) from comment #5) > > I thought perhaps this was for completeness when compared to std::optional, > > but https://en.cppreference.com/w/cpp/utility/optional says: > > > > There are no optional references; a program is ill-formed if it > > instantiates an > > optional with a reference type. > > Very interesting! > > I would like our WTF::Optional to match std::optional and I intend to switch > us off of it. You can see a brief debate in bug 211674 where Chris Dumez > suggests that our different semantic of guaranteeing a WTF::Optional is set > to WTF::nullopt when moving from it is a reason to stay with WTF::Optional. > I’d like to resolve that issue and dump WTF::Optional entirely as soon as we > can. I think we've been advocating using std::exchange() for a while now. We can probably try and switch to std::optional. Hopefully, use-after-move won't be as much as an issue this time around. > In the mean time, I suggest we minimize differences from std::optional, > which points to removing our support for optional references. +1
This is obsolete now, because I removed the contents of Optional.hhttps://bugs.webkit.org/show_bug.cgi?id=226161#