-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement P0798R8 monadic operations for std::optional #2301
Conversation
cddc871
to
ee15a99
Compare
tests/std/tests/P0798R8_monadic_operations_for_std_optional/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/P0798R8_monadic_operations_for_std_optional/test.cpp
Outdated
Show resolved
Hide resolved
tests/std/tests/P0798R8_monadic_operations_for_std_optional/test.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The requires(
to requires (
changes are "serious", others are simply suggestions.
using _Uty = invoke_result_t<_Fn, _Ty&>; | ||
|
||
static_assert(_Is_specialization_v<remove_cvref_t<_Uty>, optional>, | ||
"optional<T>::and_then(F) requires the return type of F to be a specialization of optional " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the remove_cvref_t
, this is more "requires the type of f(value())
(where f
denotes the argument to and_then
) to be a cv-qualified specialization of optional
" but that's too precise to be clear to the people who actually need this message. Maybe "requires the argument to return a specialization of optional
"?
No change required if you don't feel it's an improvement, but if you do please change all four places consistently.
using _Uty = remove_cv_t<invoke_result_t<_Fn, _Ty&>>; | ||
|
||
static_assert(!_Is_any_of_v<_Uty, nullopt_t, in_place_t>, | ||
"optional<T>::transform(F) requires the return type of F to be a type other than nullopt_t or in_place_t " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically "other than cv-nullopt_t
or cv-in_place_t
", but that's strictly worse. (Please don't change.)
I decided to simply push a commit to correct a couple of syntax nits and verify preconditions of a test function. I'll move this to Ready to Merge, but feel free to address the pure suggestion comments I left if you like. |
I'll add this to the PR mirror that I'm preparing now. If cleanup changes are desired, they can go into a followup PR. Please inform me if any further changes are pushed to this PR, or if I should exclude it from this batch for any reason. |
Pushed a merge with |
Thanks for implementing this C++23 feature! 😻 ✅ 🚀 |
Implements Monadic Operations for
std::optional
as specified in [optional.monadic].Fixes #2242
I applied(edit: removed after code review)_NODISCARD
to all overloads, since they all return a value, though it's possible that someone wants to use them for control flow and ignore the return value.