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. ```
I found 116 instances in WebKit where we can replace this: ``` optional.value_or(InnerType { }) ``` ...with this: ``` valueOrDefault(optional) ```
Created attachment 448393 [details] For EWS
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 { }); }
(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!
Created attachment 448467 [details] Patch
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 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.
Could probably still use ? : instead of an if statement.
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.
(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.
Created attachment 448527 [details] Patch for landing
(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
Created attachment 448533 [details] For EWS
(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 🤦🏻♂️
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].
<rdar://problem/87224711>