diff options
author | Hui <hui.xie1990@gmail.com> | 2024-06-26 12:10:59 +0100 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-06-26 12:10:59 +0100 |
commit | 16f349251fabacfdba4acac3b25baf0e6890c1a0 (patch) | |
tree | d4e15221460cabf87440f7b2e6d9bbecaf336a31 | |
parent | f782ff8fc6426890863be0791a9ace2394da3887 (diff) | |
download | llvm-16f349251fabacfdba4acac3b25baf0e6890c1a0.zip llvm-16f349251fabacfdba4acac3b25baf0e6890c1a0.tar.gz llvm-16f349251fabacfdba4acac3b25baf0e6890c1a0.tar.bz2 |
[libc++] restrict the expected conversion constructor not compete against copy constructor (#96101)
fixes #92676
So right now clang does not like
```
std::expected<std::any, int> e1;
auto e2 = e1;
```
So basically when clang tries to do overload resolution of `auto e2 =
e1;`
It finds
```
expected(const expected&); // 1. This is OK
expected(const expected<_Up, _OtherErr>&) requires __can_convert; // 2. This needs to check its constraints
```
Then in `__can_convert`, one of the check is
```
_Not<is_constructible<_Tp, expected<_Up, _OtherErr>&>>
```
which is checking
```
is_constructible<std::any, expected<_Up, _OtherErr>&>
```
Then it looks at `std::any`'s constructor
```
template < class _ValueType,
class _Tp = decay_t<_ValueType>,
class = enable_if_t< !is_same<_Tp, any>::value && !__is_inplace_type<_ValueType>::value &&
is_copy_constructible<_Tp>::value> >
any(_ValueType&& __value);
```
In the above, `is_copy_constructible<_Tp>` expands to
```
is_copy_constructible<std::expected<std::any, int>>
```
And the above goes back to the original thing we asked : copy the
`std::expected`, which goes to the overload resolution again.
```
expected(const expected&);
expected(const expected<_Up, _OtherErr>&) requires __can_convert;
```
So the second overload results in a logical cycle.
I am not a language lawyer. We could argue that clang should give up on
the second overload which has logical cycle, as the first overload is a
perfect match.
Anyway, the fix in this patch tries to short-circuiting the second
overload's constraint check: that is, if the argument matches exact same
`expected<T, E>`, we give up immediately and let the copy constructor to
deal with it
-rw-r--r-- | libcxx/include/__expected/expected.h | 4 | ||||
-rw-r--r-- | libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.copy.pass.cpp | 20 |
2 files changed, 23 insertions, 1 deletions
diff --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h index 0f994e2..f618b20 100644 --- a/libcxx/include/__expected/expected.h +++ b/libcxx/include/__expected/expected.h @@ -507,7 +507,9 @@ private: _And< is_constructible<_Tp, _UfQual>, is_constructible<_Err, _OtherErrQual>, _If<_Not<is_same<remove_cv_t<_Tp>, bool>>::value, - _And< _Not<is_constructible<_Tp, expected<_Up, _OtherErr>&>>, + _And< + _Not<_And<is_same<_Tp, _Up>, is_same<_Err, _OtherErr>>>, // use the copy constructor instead, see #92676 + _Not<is_constructible<_Tp, expected<_Up, _OtherErr>&>>, _Not<is_constructible<_Tp, expected<_Up, _OtherErr>>>, _Not<is_constructible<_Tp, const expected<_Up, _OtherErr>&>>, _Not<is_constructible<_Tp, const expected<_Up, _OtherErr>>>, diff --git a/libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.copy.pass.cpp b/libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.copy.pass.cpp index 581df51..ba98317 100644 --- a/libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.copy.pass.cpp +++ b/libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.copy.pass.cpp @@ -62,6 +62,16 @@ static_assert(!std::is_trivially_copy_constructible_v<std::expected<CopyableNonT static_assert(!std::is_trivially_copy_constructible_v<std::expected<int, CopyableNonTrivial>>); static_assert(!std::is_trivially_copy_constructible_v<std::expected<CopyableNonTrivial, CopyableNonTrivial>>); +struct Any { + constexpr Any() = default; + constexpr Any(const Any&) = default; + constexpr Any& operator=(const Any&) = default; + + template <class T> + requires(!std::is_same_v<Any, std::decay_t<T>> && std::is_copy_constructible_v<std::decay_t<T>>) + constexpr Any(T&&) {} +}; + constexpr bool test() { // copy the value non-trivial { @@ -109,6 +119,16 @@ constexpr bool test() { assert(!e2.has_value()); } + { + // TODO(LLVM 20): Remove once we drop support for Clang 17 +#if defined(TEST_CLANG_VER) && TEST_CLANG_VER >= 1800 + // https://github.com/llvm/llvm-project/issues/92676 + std::expected<Any, int> e1; + auto e2 = e1; + assert(e2.has_value()); +#endif + } + return true; } |