Modify constructor constraints to make 'optional<any>' work on libc++#99
Conversation
jiixyj
requested review from
camio,
neatudarius and
steve-downey
as code owners
steve-downey
left a comment
steve-downey
left a comment
There was a problem hiding this comment.
Thanks for these and related PRs! I will take a look at all of them in the next few days.
steve-downey
left a comment
steve-downey
left a comment
There was a problem hiding this comment.
I've reproduced this against a few versions of clang/libc++. Older versions fail instead on !std::is_convertible_v<optional<U>&, T> && not being a constant expression, which I'm pretty sure is a bug in libc++.
Still making sure I understand the changes in enable_from_other. I may have been smarter when I wrote that.
| !std::is_constructible_v<T, const optional<U>&&> && !std::is_convertible_v<optional<U>&, T> && | ||
| !std::is_convertible_v<optional<U>&&, T> && !std::is_convertible_v<const optional<U>&, T> && | ||
| !std::is_convertible_v<const optional<U>&&, T>; | ||
| !std::is_same_v<T, U> && std::is_constructible_v<T, Other> && // |
There was a problem hiding this comment.
I don't think the // at the end of line needs to be there?
There was a problem hiding this comment.
Yes, it doesn't need to be there. Sometimes I do this out of habit to force clang-format to break a line. In this case it makes it so that the next two lines are the is_constructible_vs and the two lines after that are the "matching" is_convertible_vs.
Should I remove it? The diff would certainly look nicer.
There was a problem hiding this comment.
I think removing it will make the diff look a little cleaner. I'm making a note to come back to this to clean up formatting, or refactor into smaller concepts, so it's more obvious what is intended and how they fit together.
The primary template was really, originally, just to have a base case for the optional<T&> part and I didn't expect to have to revisit it.
There was a problem hiding this comment.
OK, I reverted the formatting change and merged main.
There was a problem hiding this comment.
Would it be OK to put you in the acknowledgements for the next rev of the paper? It's going out in the next few days and hopefully reviewed and forwarded for C++26 in a few weeks.
There was a problem hiding this comment.
Would it be OK to put you in the acknowledgements for the next rev of the paper? It's going out in the next few days and hopefully reviewed and forwarded for C++26 in a few weeks.
Sure, thank you!
3 participants
For
enable_forward_value, I just reordered the constraints so that the check againstoptionalcomes first. This fixes a "satisfaction of constraint 'detail::enable_forward_value<T, U>' depends on itself" error on clang.For
enable_from_otherI introduced a!std::is_same_v<T, U>as the first check. This also fixes a "satisfaction of constraint ... depends on itself" error. This check should lead to no change in behavior as in the case whereTandUare the same,optional<T>'s copy/move constructors should be used anyway.