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

[Yurong via Phabricator via libcxx-commits]

yronglin marked 3 inline comments as done.
yronglin added a comment.

Thanks for your comments! @philnik



================
Comment at: libcxx/include/__expected/expected.h:906
+    // the expected's destructor handles this
+    _LIBCPP_HIDE_FROM_ABI constexpr ~__union_t() {}
+
----------------
Also removed required here.


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


================
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:
> 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.
Remove `-verify-ignore-unexpected=error` and checked more errors.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911