[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:719-722
+// [iterator.traits]/3.4
+// Otherwise, `iterator_traits<I>` has no members by any of the above names.
+template<class>
+struct __iterator_traits {};
----------------
zoecarver wrote:
> cjdb wrote:
> > I'd prefer that we forward declare `__iterator_traits` and then define it under [iterator.traits]/3.3. That's what we did for [[ https://reviews.llvm.org/D96657#inline-913994 | `common_reference` ]], and I think that helped improve readability. Similarly elsewhere.
> IMHO forward declaring types when we don't have to introduces a lot of complexity and makes the implementation much more difficult to read. Especially if I'm looking for the definition of something later. 
> 
> I wasn't a reviewer on that patch, but if I were I would have pushed for a different implementation, or at least different names. I find it super confusing to have `__common_ref` forward declared and then used before being defined. Not to mention the presence of the following names `__common_reference_sub_bullet{1,2,3}` and `__common_ref_{C,D}`. When future me is trying to find the implementation of something in libc++, I'd rather not have to do the archeology to decode such names, and having them be forward declared, make it that much more difficult to find the actual implementation. 
> 
> (Side note: I think we should really discourage naming thing after bullets in the standard. Those change fairly frequently, and the names of things in libc++ hardly ever change. Not only does it make things harder to read/understand, but will likely become incorrect soon.)
> 
> However, I agree that this could be made more readable. WDYT about removing `__iterator_traits` (I don't see any reason we can't just implement `iterator_traits` directly), and moving all the "implementation details" above so that we can just have one specialization after another. I know this still means we have to put the final "Otherwise" first, but I don't think that adds any complexity, it's the natural way to implement something (I'm sure when creating concepts the committee didn't expect everyone to define the "default" case last using a forward declared class).
> WDYT about removing `__iterator_traits` (I don't see any reason we can't just implement `iterator_traits` directly), and moving all the "implementation details" above so that we can just have one specialization after another.

@tcanens made a comment early on in this patch's review pointing out that the primary template mustn't conflict with others' specialisations.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:49-57
+
+using VectorIteratorTraits = std::iterator_traits<std::vector<int>::iterator>;
+static_assert(std::same_as<VectorIteratorTraits::iterator_category,
+                           std::random_access_iterator_tag>);
+static_assert(std::same_as<VectorIteratorTraits::value_type, int>);
+static_assert(
+    std::same_as<VectorIteratorTraits::difference_type, std::ptrdiff_t>);
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > cjdb wrote:
> > > cjdb wrote:
> > > > I think these should be `std::same_as<VectorIteratorTraits::member, std::vector<int>::iterator::member>`. I don't particularly care about the concrete types they resolve to, but I do care that they match the members of said iterator.
> > > > 
> > > > I also notice there's no test for whether or not `vector` iterators have `iterator_concept`.
> > > It'd be great if you could section off each subset of tests with the paragraph numbers they correspond to.
> > I do care what they resolve to. Leave them as-is, i.e., `static_assert(std::same_as<VectorIteratorTraits::value_type, int>);` is the best way to write it. (Although, if you want to name the typedef `VIT` to save on column-width and thus fix that one ugly linebreak, that'd be a good idea.)
> > I also notice there's no test for whether or not vector iterators have iterator_concept.
> 
> I'll add some tests. 
> 
> > I do care what they resolve to. Leave them as-is, i.e., static_assert(std::same_as<VectorIteratorTraits::value_type, int>); is the best way to write it.
> 
> I agree. I think it makes more sense to use the concrete types. 
> 
> > (Although, if you want to name the typedef VIT to save on column-width and thus fix that one ugly linebreak, that'd be a good idea.)
> 
> I thought clang format wasn't going to generate line breaks anymore...
>> I do care what they resolve to. Leave them as-is, i.e., static_assert(std::same_as<VectorIteratorTraits::value_type, int>); is the best way to write it.
>
> I agree. I think it makes more sense to use the concrete types.

In that case, keep them, and please add the tests I've requested. It's important that `VectorIteratorTraits::member` //is// `std::vector<int>::iterator::member`.

>> (Although, if you want to name the typedef VIT to save on column-width and thus fix that one ugly linebreak, that'd be a good idea.)
>
> I thought clang format wasn't going to generate line breaks anymore...

Cryptic names hurt readability far more than benign whitespace.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855