-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[libc++] Implement LWG3940: std::expected<void, E>::value() also needs E to be copy constructible #71819
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
Conversation
…s E to be copy constructible)
@llvm/pr-subscribers-libcxx Author: Twice (PragmaTwice) ChangesThis patch includes the fix for LWG3940 ( TODO: maybe add more tests and check compiler error messages precisely via llvm-lit Full diff: https://github.com/llvm/llvm-project/pull/71819.diff 4 Files Affected:
diff --git a/libcxx/docs/Status/Cxx2cIssues.csv b/libcxx/docs/Status/Cxx2cIssues.csv
index a928b69ea10c489..099a19d02888f4c 100644
--- a/libcxx/docs/Status/Cxx2cIssues.csv
+++ b/libcxx/docs/Status/Cxx2cIssues.csv
@@ -15,7 +15,7 @@
"`3927 <https://wg21.link/LWG3927>`__","Unclear preconditions for ``operator[]`` for sequence containers","Varna June 2023","|Nothing To Do|","",""
"`3935 <https://wg21.link/LWG3935>`__","``template<class X> constexpr complex& operator=(const complex<X>&)`` has no specification","Varna June 2023","|Complete|","3.4",""
"`3938 <https://wg21.link/LWG3938>`__","Cannot use ``std::expected`` monadic ops with move-only ``error_type``","Varna June 2023","|Complete|","18.0",""
-"`3940 <https://wg21.link/LWG3940>`__","``std::expected<void, E>::value()`` also needs ``E`` to be copy constructible","Varna June 2023","","",""
+"`3940 <https://wg21.link/LWG3940>`__","``std::expected<void, E>::value()`` also needs ``E`` to be copy constructible","Varna June 2023","|Complete|","18.0",""
"","","","","",""
"`3343 <https://wg21.link/LWG3343>`__","Ordering of calls to ``unlock()`` and ``notify_all()`` in Effects element of ``notify_all_at_thread_exit()`` should be reversed","Not Yet Adopted","|Complete|","16.0",""
"`3892 <https://wg21.link/LWG3892>`__","Incorrect formatting of nested ranges and tuples","Not Yet Adopted","|Complete|","17.0","|format|"
diff --git a/libcxx/include/__expected/expected.h b/libcxx/include/__expected/expected.h
index bf16c8f720d2681..c6441f1651b0855 100644
--- a/libcxx/include/__expected/expected.h
+++ b/libcxx/include/__expected/expected.h
@@ -1187,12 +1187,15 @@ class expected<_Tp, _Err> {
}
_LIBCPP_HIDE_FROM_ABI constexpr void value() const& {
+ static_assert(is_copy_constructible_v<_Err>, "error_type has to be copy constructible");
if (!__has_val_) {
std::__throw_bad_expected_access<_Err>(__union_.__unex_);
}
}
_LIBCPP_HIDE_FROM_ABI constexpr void value() && {
+ static_assert(is_copy_constructible_v<_Err> && is_move_constructible_v<_Err>,
+ "error_type has to be both copy constructible and move constructible");
if (!__has_val_) {
std::__throw_bad_expected_access<_Err>(std::move(__union_.__unex_));
}
diff --git a/libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.compile.fail.cpp b/libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.compile.fail.cpp
new file mode 100644
index 000000000000000..37c06e629e9443e
--- /dev/null
+++ b/libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.compile.fail.cpp
@@ -0,0 +1,27 @@
+//===----------------------------------------------------------------------===//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
+// constexpr void value() &&;
+// Mandates: is_copy_constructible_v<E> is true and is_move_constructible_v<E> is true.
+
+#include <expected>
+
+#include "MoveOnly.h"
+
+void test() {
+ // MoveOnly
+ std::expected<void, MoveOnly> e(std::unexpect, 5);
+
+ // COMPILE FAIL: MoveOnly is not copy constructible
+ std::move(e).value();
+}
+
+int main(int, char**) {
+ test();
+}
diff --git a/libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp b/libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp
index 0f02db19c7b75e7..a24d67c57e19b59 100644
--- a/libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp
+++ b/libcxx/test/std/utilities/expected/expected.void/observers/value.pass.cpp
@@ -65,17 +65,6 @@ void testException() {
}
}
- // MoveOnly
- {
- std::expected<void, MoveOnly> e(std::unexpect, 5);
- try {
- std::move(e).value();
- assert(false);
- } catch (const std::bad_expected_access<MoveOnly>& ex) {
- assert(ex.error() == 5);
- }
- }
-
#endif // TEST_HAS_NO_EXCEPTIONS
}
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.compile.fail.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.
LGTM with all issues are addressed. Thank you!
libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.verify.cpp
Outdated
Show resolved
Hide resolved
libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.verify.cpp
Outdated
Show resolved
Hide resolved
@@ -65,17 +65,6 @@ void testException() { | |||
} | |||
} | |||
|
|||
// MoveOnly |
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.
Out of interest, according to the LWG issue, this test should never work. Do you know why there was not an issue before?
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.
Yeah due to LWG3940 it cannot pass the compiler. But I guess before this the expected::value
implementation can actually accept move-only types.
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.
Out of interest, according to the LWG issue, this test should never work. Do you know why there was not an issue before?
This is because of Clang's non-conforming acception of move-only exception types, which is reported as #53224.
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.
Why would this extension be non-conforming? A warning is missing, but other than that I don't see how it wouldn't be conforming.
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.
It seems that throw NonCopyable(args)
also needs to result in substitution failure, while Clang currently disagrees (example). See also this article.
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.
Thanks for your patch!
libcxx/test/std/utilities/expected/expected.void/observers/value.lwg3940.verify.cpp
Outdated
Show resolved
Hide resolved
It seems the CI has failed, but I'm not sure if it is related to my changes, since I don't touch these failed test cases.
|
libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp
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.
LGTM
libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp
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.
@PragmaTwice There are some review comments still open. Do you have time to address these? Then we can merge this patch.
Apologies for my delayed response. I intend to complete it this week. |
Yeah I've seen these failed cases. I will try to merge the latest main branch and rerun the CI.
|
https://github.com/llvm/llvm-project/actions/runs/7404652886/job/20147039107?pr=71819
It seems in some specific version/config of clang, these error messages do not appear. May I ask if there is any method to workaround these non-important error messages? |
libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp
Outdated
Show resolved
Hide resolved
The CI finally passed now : ) |
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 CI finally passed now : )
Great. A few minor comment and then it LGTM. I'll leave the final approval to @huixie90
libcxx/test/libcxx/utilities/expected/expected.void/value.lwg3940.verify.cpp
Outdated
Show resolved
Hide resolved
LGTM. @mordante could you please have another look if the current state addressed your concerns? |
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.
LGTM. @mordante could you please have another look if the current state addressed your concerns?
Yes looks fine to me.
This patch includes the fix for LWG3940 (
std::expected<void, E>::value()
also needsE
to be copy constructible)