[libcxx-commits] [PATCH] D140911: [libc++] Implement P2505R5(Monadic operations for std::expected).

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik accepted this revision.
philnik added a comment.

LGTM % comments.



================
Comment at: libcxx/include/__expected/expected.h:1510
+    _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t()
+      requires(!is_trivially_destructible_v<_ErrorType>)
+    {}
----------------
This shouldn't be required, since the requires clause causes the `= default`ed to be preferred.


================
Comment at: libcxx/test/libcxx/utilities/expected/expected.expected/and_then.mandates.verify.cpp:10
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=error
+
----------------
philnik wrote:
> yronglin wrote:
> > philnik wrote:
> > > This seems weird. Why should we ignore errors?
> > This used to ignore errors like `type '_Up' (aka 'int') cannot be used prior to '::' because it has no members`, `no matching constructor for initialization of '_Up' (aka 'std::expected<int, NotSameAsInt>')`, and so on, I think we don't need to check errors like that, what do you think?
> I'm not super happy with just ignoring errors, since we might miss it if they change in a relevant way. OTOH having a lot of other errors littered in between also seems like a not-so-great thing. IDK, does anybody else have thoughts here?
I think I'd rather have a few more errors here than missing something important.


================
Comment at: libcxx/test/libcxx/utilities/expected/expected.expected/transform_error.mandates.verify.cpp:11
+
+// ADDITIONAL_COMPILE_FLAGS: -Xclang -verify-ignore-unexpected=note
+
----------------
notes should normally be ignored. No need to specify this here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911