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

[Yurong via Phabricator via libcxx-commits]

yronglin added a comment.

Thanks for your review @ldionne !



================
Comment at: libcxx/include/__expected/expected.h:562
     if (!__has_val_) {
-      std::__throw_bad_expected_access<_Err>(__union_.__unex_);
+      std::__throw_bad_expected_access<_Err>(std::as_const(error()));
     }
----------------
ldionne wrote:
> I don't think it's possible to test this added `as_const`, since IIUC this is a no-op given that this member function has a `const&` qualifier. Right?
Yes, as you said, this member function has a const& qualifier.


================
Comment at: libcxx/include/__expected/expected.h:570
     if (!__has_val_) {
-      std::__throw_bad_expected_access<_Err>(__union_.__unex_);
+      std::__throw_bad_expected_access<_Err>(std::as_const(error()));
     }
----------------
ldionne wrote:
> I don't think we're testing the added `as_const` here. I think we could test this by using an error type with two constructors:
> 
> ```
> Error(Error const&);
> Error(Error&);
> ```
> 
> We'd make sure that the first one got called, not the second one. Does that make sense?
Agree. I have add a test case to check this.


================
Comment at: libcxx/test/libcxx/utilities/expected/expected.expected/value.observers.verify.cpp:123-128
+#ifndef _LIBCPP_HAS_NO_EXCEPTIONS
+  // expected-error-re@*:* {{call to deleted constructor of{{.*}}}}
+  // expected-error-re@*:* {{call to deleted constructor of{{.*}}}}
+  // expected-error-re@*:* {{call to deleted constructor of{{.*}}}}
+  // expected-error-re@*:* {{call to deleted constructor of{{.*}}}}
+#endif
----------------
ldionne wrote:
> Right?
Yes!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154110