[libcxx-commits] [PATCH] D154110: [libc++] Implement LWG3843 (std::expected<T, E>::value() & assumes E is copy constructible)

[Yurong via Phabricator via libcxx-commits]

yronglin marked an inline comment as done.
yronglin added a comment.

Thanks for your review @ldionne !



================
Comment at: libcxx/test/std/utilities/expected/expected.expected/observers/value.pass.cpp:23-39
+template <typename E>
+concept has_value =
+    requires(E&& e) {
+      { std::forward<E>(e).value() };
+    };
+
+struct OnlyCopyConstructibleErrorType {
----------------
ldionne wrote:
> I don't think this tests what you intend to test. If you remove `as_const` in the `&` overload, does this test suddenly fail? I don't think it would, since you're still providing the declaration of the member function.
> 
> Instead, I think what you want is this:
> 
> ```
> bool ref_called = true;
> bool const_ref_called = true;
> 
> struct Foo {
>   Foo(Foo const&) { const_ref_called = true; }
>   Foo(Foo&) { ref_called = true; }
> };
> 
> int main() {
>   std::expected<int, Foo> e;
>   try {
>     e.value();
>   catch (std::bad_expected_access const&) {
>     assert(const_ref_called);
>   }
> }
> ```
> 
> Obviously this is a poor way to test this, but I think you see the point I'm trying to make?
> 
Good catch! It's my mistake, I have update the test case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154110/new/

https://reviews.llvm.org/D154110