Skip to content
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

Merged
merged 5 commits into from
Dec 17, 2021

Conversation

cpplearner
Copy link
Contributor

@cpplearner cpplearner commented Oct 25, 2021

Implements Monadic Operations for std::optional as specified in [optional.monadic].

Fixes #2242

I applied _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. (edit: removed after code review)

@cpplearner cpplearner requested a review from a team as a code owner October 25, 2021 18:07
stl/inc/optional Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter added the cxx23 C++23 feature label Oct 25, 2021
@cpplearner cpplearner force-pushed the monadic-op branch 2 times, most recently from cddc871 to ee15a99 Compare October 25, 2021 18:59
stl/inc/optional Outdated Show resolved Hide resolved
stl/inc/optional Outdated Show resolved Hide resolved
stl/inc/optional Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej self-assigned this Nov 3, 2021
stl/inc/optional Outdated Show resolved Hide resolved
stl/inc/optional Outdated Show resolved Hide resolved
stl/inc/optional Show resolved Hide resolved
stl/inc/optional Outdated Show resolved Hide resolved
stl/inc/optional Outdated Show resolved Hide resolved
stl/inc/optional Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej removed their assignment Nov 19, 2021
@StephanTLavavej StephanTLavavej self-assigned this Nov 19, 2021
Copy link
Member

@CaseyCarter CaseyCarter left a 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.

stl/inc/optional Outdated Show resolved Hide resolved
stl/inc/optional Outdated Show resolved Hide resolved
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 "
Copy link
Member

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 "
Copy link
Member

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.)

@CaseyCarter CaseyCarter assigned cpplearner and unassigned mnatsuhara Dec 16, 2021
@CaseyCarter
Copy link
Member

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.

@StephanTLavavej StephanTLavavej self-assigned this Dec 16, 2021
@StephanTLavavej
Copy link
Member

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.

@StephanTLavavej
Copy link
Member

Pushed a merge with main to fix a trivial adjacent-add conflict in the feature-test macros test. (The yvals_core.h feature test macros merged silently in unsorted order; I'll fix that up when merging #2350 next - there's no harm except style.)

@StephanTLavavej StephanTLavavej merged commit 20b8f61 into microsoft:main Dec 17, 2021
@StephanTLavavej
Copy link
Member

Thanks for implementing this C++23 feature! 😻 ✅ 🚀

@cpplearner cpplearner deleted the monadic-op branch December 17, 2021 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cxx23 C++23 feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P0798R8 Monadic Operations For optional
6 participants
  翻译: