[libcxx-commits] [PATCH] D99854: [libcxx] adds `cpp17-.*iterator` concepts (as `__legacy_.*iterator`)

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a comment.

Please mark LWG3420 as complete in the status. Haven't looked at the tests yet.



================
Comment at: libcxx/include/iterator:541
+template<__dereferenceable _Tp>
+using iter_reference_t = decltype(*declval<_Tp&>());
 #endif // !defined(_LIBCPP_HAS_NO_RANGES)
----------------
I don't see any tests for `iter_reference_t`. I know it's used in the other implementations so no need to test it heavily, but a simple test file would probably be good. 


================
Comment at: libcxx/include/iterator:652
 
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_CONCEPTS)
+
----------------
How about `_LIBCPP_HAS_NO_RANGES`?


================
Comment at: libcxx/include/iterator:706
+  totally_ordered<_Ip> and
+  requires(_Ip __i, _Ip __j, typename incrementable_traits<_Ip>::difference_type __n) {
+    { __i += __n } -> same_as<_Ip&>;
----------------
Why do we need `__j` too?


================
Comment at: libcxx/include/iterator:712
+    { __i -  __n } -> same_as<_Ip>;
+    { __i -  __j } -> same_as<decltype(__n)>;
+    {  __i[__n]  } -> convertible_to<iter_reference_t<_Ip>>;
----------------
This could just be `__i - __i` which is what it says in the standard anyway. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99854