[libcxx-commits] [PATCH] D99873: [libcxx] adds `std::ranges::iter_move` and `std::iter_rvalue_reference_t

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Just some super small things. But LGTM!



================
Comment at: libcxx/include/concepts:452
+template<class _Tp>
+concept __lvalue_reference = __core_reference<_Tp> && is_lvalue_reference_v<_Tp>;
+
----------------
Can't this just be ` = is_lvalue_reference_v<_Tp>;`?


================
Comment at: libcxx/include/concepts:455
+template<class _Tp>
+concept __rvalue_reference = __core_reference<_Tp> && !__lvalue_reference<_Tp>;
+
----------------
Same as above but for rvalue reference. Then we could get rid of `__core_reference`.


================
Comment at: libcxx/include/concepts:457
+
+#endif // _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
 
----------------
Nit: want to use your new macro here ;) ?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp:24
+#include "../unqualified_lookup_wrapper.h"
+
+// Wrapper around an iterator for testing `iter_move` when an unqualified call to `iter_move` isn't
----------------
Given the discussion about unions and enums, want to add a super simple type traits test for unqualified lookup on both an enum and union?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp:95
+  static_assert(!noexcept(std::ranges::iter_move(first)),
+                "unqualified-lookup case not being chosen");
+
----------------
I'm OK with this implementation. You might consider a static global to track this stuff in the future, though. Especially if you need more than a boolean. As I said, though, I'm OK with this implementation here. 


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp:138
+
+constexpr bool check_iter_move() {
+  constexpr int full_size = 100;
----------------
I think this is a really good test!


================
Comment at: libcxx/test/std/iterators/iterator.requirements/iterator.cust/iterator.cust.move/iter_move.pass.cpp:186
+  static_assert(check_iter_move());
+  assert(check_iter_move());
+
----------------
No need to call assert here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99873