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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Thanks for the patch! I really appreciate the added `.verify.pass.cpp` tests. I have a few questions but this basically LGTM.



================
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()));
     }
----------------
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?


================
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()));
     }
----------------
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?


================
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
----------------
Right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154110