[libcxx-commits] [PATCH] D155701: [libc++][expected] Implement LWG3836

[Yurong via Phabricator via libcxx-commits]

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

Thanks a lot for your review! @Mordante



================
Comment at: libcxx/include/__expected/expected.h:171-180
+            _Or< is_same<remove_cv_t<_Tp>, bool>,
+                 _And< _Not<is_constructible<_Tp, expected<_Up, _OtherErr>&>>,
+                       _Not<is_constructible<_Tp, expected<_Up, _OtherErr>>>,
+                       _Not<is_constructible<_Tp, const expected<_Up, _OtherErr>&>>,
+                       _Not<is_constructible<_Tp, const expected<_Up, _OtherErr>>>,
+
+                       _Not<is_convertible<expected<_Up, _OtherErr>&, _Tp>>,
----------------
Mordante wrote:
> Why not something like this? This looks closer to the wording in the issue. It saves the reader from doing mental boolean calculations when comparing the code against the wording in the standard.
> 
> I would also more the new condition down so again it's closer to the wording of the standard. The new paragraph 23.5 is also added at the end and not in between.
Seems the `converts-from-any-cvref<T, expected<U, G>> is false` only performed when `if T is not cv bool` it true, we can't put  `s_same<remove_cv_t<_Tp>, bool>` and `converts-from-any-cvref<T, expected<U, G>>` in the same conjunction expression, because the result was not correct. Can we use `_If` here?, it's more closing to the wording. WDYT?


================
Comment at: libcxx/include/__expected/expected.h:228
     requires(!is_same_v<remove_cvref_t<_Up>, in_place_t> && !is_same_v<expected, remove_cvref_t<_Up>> &&
-             !__is_std_unexpected<remove_cvref_t<_Up>>::value && is_constructible_v<_Tp, _Up>)
+             !__is_std_unexpected<remove_cvref_t<_Up>>::value && is_constructible_v<_Tp, _Up> &&
+             // LWG3836
----------------
Mordante wrote:
> Not your change but this matches the order of the wording in the WP.
Thanks, this looks good, done!


================
Comment at: libcxx/test/std/utilities/expected/expected.expected/ctor/ctor.u.pass.cpp:123
     assert(e2.has_value());
-    assert(e2.value()); // yes, e2 holds "true"
+    assert(!e2.value()); // yes, e2 holds "false" since LWG3836
   }
----------------
Mordante wrote:
> There are 5 changes in the wording, can you make sure there are tests for all changes?
Thanks for your tips, I have add more test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155701