Bug 226161 – typo in `Optional<T&>::value() const` prevents usage of `Optional<const T&>`

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: Web Template Framework

[Cameron McCormack (:heycam)]

.

[Cameron McCormack (:heycam)]

Created attachment 429502 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 429502 [details]
Patch

Please add a test. (TestWTF)

[Cameron McCormack (:heycam)]

Created attachment 429503 [details]
Patch

[Myles C. Maxfield]

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.

[Cameron McCormack (:heycam)]

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.

[Darin Adler]

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.

[Chris Dumez]

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

[Darin Adler]

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

[Chris Dumez]

(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

[Darin Adler]

This is obsolete now, because I removed the contents of Optional.hhttps://bugs.webkit.org/show_bug.cgi?id=226161#