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

[Yurong via Phabricator via libcxx-commits]

yronglin added a comment.

Thank you for your comment @philnik .  For GCC's bug, I have applied for an gcc bugzilla account and am waiting for their reply.



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

+1 looks good, I'll update patch later!


================
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");
----------------
philnik wrote:
> Maybe `__error` to make clear what this is? Just `__v` isn't very expressive.
> Maybe `__error` to make clear what this is? Just `__v` isn't very expressive.

+1, I'll update later.


================
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:
> philnik wrote:
> > 
> Could you add `.verify.cpp` tests for the `static_assert`s in `libcxx/test/libcxx`?
> Could you add `.verify.cpp` tests for the `static_assert`s in `libcxx/test/libcxx`?

Ok, I'll do that.


================
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&>>;
----------------
philnik wrote:
> This formatting seems very weird. Does clang-format produce this? If yes, which version are you using? Maybe you have to upgrade.
> This formatting seems very weird. Does clang-format produce this? If yes, which version are you using? Maybe you have to upgrade.

Yes, I think so too, when I use vscode + clangd plugin, the formatted code looks like:
```
  // [expected.void.monadic], monadic
  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&>>;
    static_assert(
        __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");
    if (has_value()) {
      return std::invoke(std::forward<_Func>(__f), value());
    }
    return _Up(unexpect, error());
  }
```
but this code can not pass CI `Format` check, therefore, according to the error message generated by CI, I manually modified the format of the code.
my clangd version is:
```
➜  llvm-project git:(main) ✗ clangd --version
clangd version 16.0.0 (https://mirrors.tuna.tsinghua.edu.cn/git/llvm-project.git 2f08872d81fd324bf3532e0919f256d475f21729)
Features: mac+xpc
Platform: arm64-apple-darwin21.6.0
```


================
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()) {
----------------
philnik wrote:
> WDYT?
> WDYT?

+1, this looks good, I'll update it later, thanks!


================
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>();
----------------
philnik wrote:
> This brings it closer to the wording again. Same  for the other overloads.
> This brings it closer to the wording again. Same  for the other overloads.

+1, I'll update it later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140911