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

[Yurong via Phabricator via libcxx-commits]

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

Many thanks for your comments @philnik !



================
Comment at: libcxx/include/__expected/expected.h:68
+
 namespace __expected {
 
----------------
philnik wrote:
> huixie90 wrote:
> > philnik wrote:
> > > @huixie90 Why do we have this namespace? It doesn't really seem worth it just for `__throw_bad_expected_access`.
> > The honest answer is i can't remember. perhaps in some point of time I had several functions but got deleted in the end. But it seems that after this patch, there is more stuff in it.
> I would remove the namespace then. It doesn't really improve the readability in any way, since every single member has `expected` already in it's name. We also don't have these kinds of detail namespaces normally.
If we decided to remove __expected namespace, I think __unexpected namespace in unexpected.h also should be removed, What do you think?


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


================
Comment at: libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp:17
+// template<class F> constexpr auto and_then(F&& f) const &&;
+
+#include <cassert>
----------------
huixie90 wrote:
> could you please test 
> > Constraints:  is_copy_constructible_v<E> is true.
> 
> Usually we have positive and negative `static_assert` to test whether or not the function exists
> 
> This applies to all functions with "Constraints"
+1


================
Comment at: libcxx/test/std/utilities/expected/expected.expected/monadic/and_then.pass.cpp:1-6
+//===----------------------------------------------------------------------===//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
----------------
philnik wrote:
> Same for the other ones.
+1


================
Comment at: libcxx/test/std/utilities/expected/expected.void/observers/error_or.pass.cpp:93
+  test_default_template_arg();
+  static_assert(test_default_template_arg());
+
----------------
philnik wrote:
> Why is this only `static_assert`ed?
Could you please clarify?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911