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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

I haven't looked at the tests yet. The implementation looks mostly good. Re. the GCC bug, I forgot to mention that you should add a link to the bug when you disable the test.



================
Comment at: libcxx/docs/Status/Cxx2bPapers.csv:95
 "`P2599R2 <https://wg21.link/P2599R2>`__","LWG","``mdspan::size_type`` should be ``index_type``","July 2022","",""
 "`P2604R0 <https://wg21.link/P2604R0>`__","LWG","mdspan: rename pointer and contiguous","July 2022","",""
 "`P2613R1 <https://wg21.link/P2613R1>`__","LWG","Add the missing ``empty`` to ``mdspan``","July 2022","",""
----------------
Please update `__cpp_lib_expected` in `libcxx/utils/generate_feature_test_macro_components.py` to `202211`.


================
Comment at: libcxx/include/__expected/expected.h:68
+
 namespace __expected {
 
----------------
@huixie90 Why do we have this namespace? It doesn't really seem worth it just for `__throw_bad_expected_access`.


================
Comment at: libcxx/include/__expected/expected.h:71
+template <class _Tp>
+struct __is_expected : false_type {};
+
----------------
Just `__is_expected` seems a bit ambiguous.


================
Comment at: libcxx/include/__expected/expected.h:622
+  template <class _Up = _Err>
+  _LIBCPP_HIDE_FROM_ABI constexpr _Err error_or(_Up&& __v) const& {
+    static_assert(is_copy_constructible_v<_Err>, "error_type has to be move constructible");
----------------
Maybe `__error` to make clear what this is? Just `__v` isn't very expressive.


================
Comment at: libcxx/include/__expected/expected.h:623
+  _LIBCPP_HIDE_FROM_ABI constexpr _Err error_or(_Up&& __v) const& {
+    static_assert(is_copy_constructible_v<_Err>, "error_type has to be move constructible");
+    static_assert(is_convertible_v<_Up, _Err>, "argument has to be convertible to error_type");
----------------



================
Comment at: libcxx/include/__expected/expected.h:623
+  _LIBCPP_HIDE_FROM_ABI constexpr _Err error_or(_Up&& __v) const& {
+    static_assert(is_copy_constructible_v<_Err>, "error_type has to be move constructible");
+    static_assert(is_convertible_v<_Up, _Err>, "argument has to be convertible to error_type");
----------------
philnik wrote:
> 
Could you add `.verify.cpp` tests for the `static_assert`s in `libcxx/test/libcxx`?


================
Comment at: libcxx/include/__expected/expected.h:625
+    static_assert(is_convertible_v<_Up, _Err>, "argument has to be convertible to error_type");
+    return __has_val_ ? static_cast<_Err>(std::forward<_Up>(__v)) : __union_.__unex_;
+  }
----------------
Let's use the public API here to match the wording. You can't `static_cast` here because that might call the wrong function: https://godbolt.org/z/TjPK9YWac. Please add a regression test. Same for the rvalue overload.


================
Comment at: libcxx/include/__expected/expected.h:637
+  template <class _Func>
+  requires is_copy_constructible_v<_Err> _LIBCPP_HIDE_FROM_ABI constexpr auto and_then(_Func&& __f) & {
+    using _Up = remove_cvref_t<invoke_result_t<_Func, _Tp&>>;
----------------
This formatting seems very weird. Does clang-format produce this? If yes, which version are you using? Maybe you have to upgrade.


================
Comment at: libcxx/include/__expected/expected.h:641
+        __expected::__is_expected<_Up>::value, "result of f(value()) must be a specialization of std::expected");
+    static_assert(is_same_v<typename _Up::error_type, _Err>,
+                  "std::remove_cvref_t<decltype(f(value()))>>::error_type must same as error_type");
----------------
This `typename` shouldn't be required anymore. Is it to support older Clang versions?


================
Comment at: libcxx/include/__expected/expected.h:642
+    static_assert(is_same_v<typename _Up::error_type, _Err>,
+                  "std::remove_cvref_t<decltype(f(value()))>>::error_type must same as error_type");
+    if (has_value()) {
----------------
WDYT?


================
Comment at: libcxx/include/__expected/expected.h:745-754
+    if (has_value()) {
+      if constexpr (!is_void_v<_Up>) {
+        return expected<_Up, _Err>(
+            __expected::__expected_construct_in_place_from_invoke_tag{}, std::forward<_Func>(__f), value());
+      } else {
+        std::invoke(std::forward<_Func>(__f), value());
+        return expected<_Up, _Err>();
----------------
This brings it closer to the wording again. Same  for the other overloads.


================
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
+//
+//===----------------------------------------------------------------------===//
----------------
Same for the other ones.


================
Comment at: libcxx/test/std/utilities/expected/expected.void/monadic/transform_error.pass.cpp:116
+  static_assert(test());
+  return 0;
+}
----------------
Let's add an empty line before the `return 0;` everywhere to make it consistent with other tests.


================
Comment at: libcxx/test/std/utilities/expected/expected.void/observers/error_or.pass.cpp:56
+  static_assert(test());
+}
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911