Bug 234865 - Add a helper function that returns the value of a std::optional<T> or constructs T if needed
Summary: Add a helper function that returns the value of a std::optional<T> or constru...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-04 16:24 PST by Wenson Hsieh
Modified: 2022-01-06 16:21 PST (History)
35 users (show)

See Also:


Attachments
For EWS (96.38 KB, patch)
2022-01-05 08:52 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (95.62 KB, patch)
2022-01-05 21:11 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch for landing (95.59 KB, patch)
2022-01-06 12:40 PST, Wenson Hsieh
wenson_hsieh: commit-queue+
Details | Formatted Diff | Diff
For EWS (95.60 KB, patch)
2022-01-06 13:28 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2022-01-04 16:24:56 PST
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.
```
Comment 1 Wenson Hsieh 2022-01-04 17:18:31 PST
I found 116 instances in WebKit where we can replace this:

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

...with this:

```
valueOrDefault(optional)
```
Comment 2 Wenson Hsieh 2022-01-05 08:52:03 PST
Created attachment 448393 [details]
For EWS
Comment 3 Fujii Hironori 2022-01-05 16:19:14 PST
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 { });
}
Comment 4 Wenson Hsieh 2022-01-05 21:09:50 PST
(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!
Comment 5 Wenson Hsieh 2022-01-05 21:11:53 PST
Created attachment 448467 [details]
Patch
Comment 6 Darin Adler 2022-01-06 09:29:49 PST
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.
Comment 7 Wenson Hsieh 2022-01-06 09:32:55 PST
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.
Comment 8 Darin Adler 2022-01-06 09:37:13 PST
Could probably still use ? : instead of an if statement.
Comment 9 Darin Adler 2022-01-06 09:38:22 PST
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.
Comment 10 Wenson Hsieh 2022-01-06 12:38:26 PST
(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.
Comment 11 Wenson Hsieh 2022-01-06 12:40:01 PST
Created attachment 448527 [details]
Patch for landing
Comment 12 Fujii Hironori 2022-01-06 12:53:42 PST
(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
Comment 13 Wenson Hsieh 2022-01-06 13:28:09 PST
Created attachment 448533 [details]
For EWS
Comment 14 Wenson Hsieh 2022-01-06 13:35:55 PST
(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 🤦🏻‍♂️
Comment 15 EWS 2022-01-06 16:20:09 PST
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].
Comment 16 Radar WebKit Bug Importer 2022-01-06 16:21:21 PST
<rdar://problem/87224711>