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

[optional.ctor], [expected.object.ctor] Add converts-from-any-cvref #5416

Merged
merged 1 commit into from Dec 16, 2022

Conversation

jwakely
Copy link
Member

@jwakely jwakely commented Apr 26, 2022

Fixes #5205

source/utilities.tex Outdated Show resolved Hide resolved
source/utilities.tex Outdated Show resolved Hide resolved
source/utilities.tex Outdated Show resolved Hide resolved
@jwakely
Copy link
Member Author

jwakely commented Apr 26, 2022

I have an alternative fix for #5205 that adds three smaller alias templates:

--- a/source/utilities.tex
+++ b/source/utilities.tex
@@ -2918,20 +2918,29 @@ [optional.optional.general]
 other than \cv{} \tcode{in_place_t} or \cv{} \tcode{nullopt_t}
 that meets the \oldconcept{Destructible} requirements (\tref{cpp17.destructible}).
 
-\rSec3[optional.ctor]{Constructors}
-
 \pnum
-The exposition-only alias template \exposid{converts-from-any-cvref}
-is used by some constructors for \tcode{optional}.
+The exposition-only alias templates
+\exposid{construct-from-any-cvref},
+\exposid{convert-from-any-cvref}, and\brk
+\exposid{assign-from-any-cvref}
+are used by some members of \tcode{optional}.
 \begin{codeblock}
 template<class T, class W>
-using @\exposid{converts-from-any-cvref}@ // \expos
+using @\exposid{construct-from-any-cvref}@ // \expos
   = disjunction<is_constructible<T, W&>, is_constructible<T, W>,
-                is_constructible<T, const W&>, is_constructible<T, const W>,
-                is_convertible<W&, T>, is_convertible<W, T>,
+                is_constructible<T, const W&>, is_constructible<T, const W>>;
+template<class T, class W>
+using @\exposid{convert-from-any-cvref}@ // \expos
+  = disjunction<is_convertible<W&, T>, is_convertible<W, T>,
                 is_convertible<const W&, T>, is_convertible<const W, T>>;
+template<class T, class W>
+using @\exposid{assign-from-any-cvref}@ // \expos
+  = disjunction<is_assignable<T&, W&>, is_assignable<T&, W>,
+                is_assignable<T&, const W&>, is_assignable<T&, const W>>;
 \end{codeblock}
 
+\rSec3[optional.ctor]{Constructors}
+
 \indexlibraryctor{optional}%
 \begin{itemdecl}
 constexpr optional() noexcept;

This allows further simplifying optional::operator= and expected::expected(const expected<U,G>&), by removing repetitive uses of is_constructible and is_assignable.

@JohelEGP
Copy link
Contributor

Looks good. But construct, convert, and assing should all end in s. (And two spaces before \\ expos.)

source/utilities.tex Outdated Show resolved Hide resolved
@jwakely
Copy link
Member Author

jwakely commented Apr 26, 2022

Rebased to combine the review comment fixes, and change "The exposition-only alias variable template" following Casey's change to it.

source/utilities.tex Outdated Show resolved Hide resolved
Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice approach.

source/utilities.tex Outdated Show resolved Hide resolved
source/utilities.tex Outdated Show resolved Hide resolved
source/utilities.tex Outdated Show resolved Hide resolved
@tkoeppe
Copy link
Contributor

tkoeppe commented Sep 8, 2022

@jwakely I think this is back in your court?

@jwakely
Copy link
Member Author

jwakely commented Sep 9, 2022

🎾

@jwakely
Copy link
Member Author

jwakely commented Sep 9, 2022

@JohelEGP gave it a 👍 but was there any other interest in the approach sketched at #5416 (comment) ? i.e. three smaller exposition-only helpers, instead of the big converts-from-any-cvref one? The latter is what's proposed by this pull request now, and what has been reviewed.

@jwakely
Copy link
Member Author

jwakely commented Sep 9, 2022

I suppose we could do that later, as a further simplification, and just go with this for now.

@tkoeppe
Copy link
Contributor

tkoeppe commented Dec 16, 2022

@jensmaurer Final thoughts?

Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@tkoeppe tkoeppe merged commit 8c60752 into cplusplus:main Dec 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create an exposition-only helper for the std::optional copy/move ctor constraints
5 participants