RESOLVED FIXED 234865
Add a helper function that returns the value of a std::optional<T> or constructs T if needed
https://bugs.webkit.org/show_bug.cgi?id=234865
Summary Add a helper function that returns the value of a std::optional<T> or constru...
Wenson Hsieh
Reported 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. ```
Attachments
For EWS (96.38 KB, patch)
2022-01-05 08:52 PST, Wenson Hsieh
no flags
Patch (95.62 KB, patch)
2022-01-05 21:11 PST, Wenson Hsieh
no flags
Patch for landing (95.59 KB, patch)
2022-01-06 12:40 PST, Wenson Hsieh
wenson_hsieh: commit-queue+
For EWS (95.60 KB, patch)
2022-01-06 13:28 PST, Wenson Hsieh
no flags
Wenson Hsieh
Comment 1 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) ```
Wenson Hsieh
Comment 2 2022-01-05 08:52:03 PST
Fujii Hironori
Comment 3 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 { }); }
Wenson Hsieh
Comment 4 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!
Wenson Hsieh
Comment 5 2022-01-05 21:11:53 PST
Darin Adler
Comment 6 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.
Wenson Hsieh
Comment 7 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.
Darin Adler
Comment 8 2022-01-06 09:37:13 PST
Could probably still use ? : instead of an if statement.
Darin Adler
Comment 9 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.
Wenson Hsieh
Comment 10 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.
Wenson Hsieh
Comment 11 2022-01-06 12:40:01 PST
Created attachment 448527 [details] Patch for landing
Fujii Hironori
Comment 12 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
Wenson Hsieh
Comment 13 2022-01-06 13:28:09 PST
Wenson Hsieh
Comment 14 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 🤦🏻‍♂️
EWS
Comment 15 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].
Radar WebKit Bug Importer
Comment 16 2022-01-06 16:21:21 PST
Note You need to log in before you can comment on or make changes to this bug.