WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 448393
[details]
For EWS
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
Created
attachment 448467
[details]
Patch
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
Created
attachment 448533
[details]
For EWS
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
<
rdar://problem/87224711
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug