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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/include/iterator:758
+struct __iterator_traits<_Ip> {
+  using iterator_category = typename __iterator_traits_iterator_category<_Ip>::type;
+  using value_type        = typename indirectly_readable_traits<_Ip>::value_type;
----------------
zoecarver wrote:
> Again, is there any reason we can't define these //before// we use them?
C++ best practice is to define things as close as possible to their first point of use. The other specialisations don't need these, and IMO shouldn't be able to see them.


================
Comment at: libcxx/include/iterator:916
+#endif
 struct _LIBCPP_TEMPLATE_VIS iterator_traits<_Tp*>
 {
----------------
zoecarver wrote:
> This also needs a few updated: `value_type` should now remove cv, adds `iterator_concept `, adds a const specialization. Are you making those changes in another patch?
> `value_type` should now remove cv,

Already applied (see below).

> adds `iterator_concept`

Already applied (see below).

> adds a const specialization

I don't see this in [iterator.traits]?


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:82
+template <class I, class Wrapper>
+struct get_reference {
+  using type = std::iter_reference_t<Wrapper>;
----------------
zoecarver wrote:
> I feel like both this and `get_pointer` are doing the thing where you write the same implementation twice and test that they do the same thing. IMHO that's not a great way to write tests. 
This was the best I could come up with. If you're able to think of a different way to get this information, I'm all ears.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:289
+};
+static_assert(check_fails<reference_as_static_member>());
----------------
zoecarver wrote:
> I have no idea what the consensus here was... are we still doing compile.passp.cpp tests or no? If no, then we should make this a "normal" .pass test.
Consensus was to keep `.compile.pass.cpp` and for them to no longer have `main`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855