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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.

This LGTM from my perspective. I think we can go back and polish some of the naming (honestly I think names like `member_pointer_or_arrow_or_void` are difficult to read and reason about), however I don't want to hold up this important review which blocks several dependencies from making progress because of that.

This LGTM and I think we should ship this as-is unless a reviewer has strong concerns that have not been alleviated yet.



================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:49
+                           std::contiguous_iterator_tag>);
+
+using VectorIteratorTraits = std::iterator_traits<std::vector<int>::iterator>;
----------------
zoecarver wrote:
> cjdb wrote:
> > cjdb wrote:
> > > 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.
> > Ping.
> I haven't done this yet. This will take some time. I just uploaded a new diff without this change, you can review the rest of the diff, and I'll upload this part when I'm done. 
IMO, I'm not sure the cost/benefit ratio is sufficient to do it. At least it's definitely not blocking.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855