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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

I finally found time to have a closer look at the patch. In general happy but I would like more test coverage to see things are correct.

@huixie90 it would be great when you also can have a look at this patch.



================
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>>,
----------------
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.


================
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
----------------
Not your change but this matches the order of the wording in the WP.


================
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
   }
----------------
There are 5 changes in the wording, can you make sure there are tests for all changes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155701