Bug 234865 – Add a helper function that returns the value of a std::optional<T> or constructs T if needed

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Wenson Hsieh]

Motivated by the fact that you can't do the following:

```
std::optional<Foo> foo;

…

return foo.value_or({ }); // Grab the optional's value if it exists, or create and return a new Foo() if it's nullopt.
```

[Wenson Hsieh]

I found 116 instances in WebKit where we can replace this:

```
optional.value_or(InnerType { })
```

...with this:

```
valueOrDefault(optional)
```

[Wenson Hsieh]

Created attachment 448393 [details]
For EWS

[Fujii Hironori]

Comment on attachment 448393 [details]
For EWS

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

> Source/WTF/wtf/StdLibExtras.h:609
> +}

Can we use Universal Reference to unify the valueOrDefault functions?

template<typename OptionalType> auto valueOrDefault(OptionalType&& optional)
{
    return std::forward<OptionalType>(optional).value_or(typename std::remove_reference_t<OptionalType>::value_type { });
}

[Wenson Hsieh]

(In reply to Fujii Hironori from comment #3)
> Comment on attachment 448393 [details]
> For EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448393&action=review
> 
> > Source/WTF/wtf/StdLibExtras.h:609
> > +}
> 
> Can we use Universal Reference to unify the valueOrDefault functions?
> 
> template<typename OptionalType> auto valueOrDefault(OptionalType&& optional)
> {
>     return std::forward<OptionalType>(optional).value_or(typename
> std::remove_reference_t<OptionalType>::value_type { });
> }

Yes, this works and is much cleaner — thanks!

[Wenson Hsieh]

Created attachment 448467 [details]
Patch

[Darin Adler]

Comment on attachment 448467 [details]
Patch

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

> Source/WTF/wtf/StdLibExtras.h:601
> +    return std::forward<OptionalType>(optional).value_or(typename std::remove_reference_t<OptionalType>::value_type { });

It would be even better to make this more generic to work with optional-like things that are not std::optional. I like the idea of one that is based on null checking and the dereferencing operator rather than on value_or and value_type:

    if (optional)
        return std::forward<OptionalType>(optional);
    return decltype(*optional) { };

Not sure that code is exactly correct, but it’s the concept. But of course, we can make this change *after* landing an initial version, because it should not have any effect on std::optional call sites.

[Wenson Hsieh]

Comment on attachment 448467 [details]
Patch

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

Thanks for the review!

>> Source/WTF/wtf/StdLibExtras.h:601
>> +    return std::forward<OptionalType>(optional).value_or(typename std::remove_reference_t<OptionalType>::value_type { });
> 
> It would be even better to make this more generic to work with optional-like things that are not std::optional. I like the idea of one that is based on null checking and the dereferencing operator rather than on value_or and value_type:
> 
>     if (optional)
>         return std::forward<OptionalType>(optional);
>     return decltype(*optional) { };
> 
> Not sure that code is exactly correct, but it’s the concept. But of course, we can make this change *after* landing an initial version, because it should not have any effect on std::optional call sites.

Sounds good! I'll give this a try, and land this non-std::optional-specific version if it works out — otherwise, I'll work towards it in a followup.

[Darin Adler]

Could probably still use ? : instead of an if statement.

[Darin Adler]

Sorry, not sure why I wrote "still". The code was using value_or, not ? :, but somehow those hashed to the same thing in my brain.

[Wenson Hsieh]

(In reply to Darin Adler from comment #6)
> Comment on attachment 448467 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448467&action=review
> 
> > Source/WTF/wtf/StdLibExtras.h:601
> > +    return std::forward<OptionalType>(optional).value_or(typename std::remove_reference_t<OptionalType>::value_type { });
> 
> It would be even better to make this more generic to work with optional-like
> things that are not std::optional. I like the idea of one that is based on
> null checking and the dereferencing operator rather than on value_or and
> value_type:
> 
>     if (optional)
>         return std::forward<OptionalType>(optional);
>     return decltype(*optional) { };

Unfortunately, I seem to be having trouble getting the latter part to compile :(

```
struct Foo { int value { 0 }; };

…

template<typename OptionalType> auto valueOrDefault(OptionalType&& optional)
{
    if (optional)
        return std::forward<OptionalType>(optional);
    return decltype(*optional) { };
}

…

valueOrDefault(std::optional<Foo> { });
```

...with output:

```
main.cpp:31:12: error: non-const lvalue reference to type 'std::optional<Foo>::value_type' (aka 'Foo') cannot bind to an initializer list temporary
    return decltype(*optional) { };
```

I think I'll land the `std::optional`-specific version for now, and see if I can make this work in a followup.

> 
> Not sure that code is exactly correct, but it’s the concept. But of course,
> we can make this change *after* landing an initial version, because it
> should not have any effect on std::optional call sites.

[Wenson Hsieh]

Created attachment 448527 [details]
Patch for landing

[Fujii Hironori]

(In reply to Wenson Hsieh from comment #10)
> main.cpp:31:12: error: non-const lvalue reference to type
> 'std::optional<Foo>::value_type' (aka 'Foo') cannot bind to an initializer
> list temporary
>     return decltype(*optional) { };
> ```
> 

You need std::remove_reference_t. https://godbolt.org/z/fd3s54Tc3

[Wenson Hsieh]

Created attachment 448533 [details]
For EWS

[Wenson Hsieh]

(In reply to Fujii Hironori from comment #12)
> (In reply to Wenson Hsieh from comment #10)
> > main.cpp:31:12: error: non-const lvalue reference to type
> > 'std::optional<Foo>::value_type' (aka 'Foo') cannot bind to an initializer
> > list temporary
> >     return decltype(*optional) { };
> > ```
> > 
> 
> You need std::remove_reference_t. https://godbolt.org/z/fd3s54Tc3

Thanks! That's what I was missing 🤦🏻‍♂️

[EWS]

Committed r287731 (245806@main): <https://commits.webkit.org/245806@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448533 [details].

[Radar WebKit Bug Importer]

<rdar://problem/87224711>