[libcxx-commits] [PATCH] D99855: [libcxx] makes `iterator_traits` C++20-aware

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I like that you're commenting with the bullets, it makes it easy to follow. Generally LGTM with a few issues, but I think we're pretty close already.



================
Comment at: libcxx/include/iterator:703
+template<class _Ip>
+concept __has_member_reference = requires { typename _Ip::reference; };
+
----------------
I find that introducing this `__has_member_reference` intermediate concept increases complexity a bit without adding much. I think it would be fine to just use `requires { typename _Ip::reference; }` in the two places where it is used. Same for `__has_member_iterator_category`.


================
Comment at: libcxx/include/iterator:725
+  using difference_type    = typename _Ip::difference_type;
+  using pointer            = typename __iterator_traits_member_pointer<_Ip>::type;
+  using reference          = typename _Ip::reference;
----------------
For the primary template, `pointer` should be (http://eel.is/c++draft/iterator.traits#3.1):

> If the qualified-id `I​::​pointer` is valid and denotes a type, then `iterator_­traits<I>​::​pointer` names that type; otherwise, it names `void`.

We shouldn't check for `operator->()`, so using `__iterator_traits_member_pointer<_Ip>` (which checks for `operator->()`) doesn't sound correct to me. We should also add a test that catches that: define a type `T` from which we could deduce `iterator_traits<T>::pointer` using `operator->()` but not otherwise, and confirm that `iterator_traits<T>::pointer` is `void`. Before fixing this comment, the result would have been whatever's deduced by `operator->`.



================
Comment at: libcxx/include/iterator:761-771
+template<class _Ip>
+concept __has_arrow =
+  !__has_member_pointer<_Ip> &&
+  requires(_Ip& __i) {
+    __i.operator->();
+  };
+
----------------
That way `__has_arrow` makes sense on its own.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855