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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:27-37
+
+using PointerIteratorTraits = std::iterator_traits<int*>;
+static_assert(std::same_as<PointerIteratorTraits::iterator_category,
+                           std::random_access_iterator_tag>);
+static_assert(std::same_as<PointerIteratorTraits::value_type, int>);
+static_assert(
+    std::same_as<PointerIteratorTraits::difference_type, std::ptrdiff_t>);
----------------
cjdb wrote:
> cjdb wrote:
> > `libcxx/test/std/iterators/iterator.primitives/iterator.traits/pointer.pass.cpp` already covers this case.
> Ping.
Removed.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:39-48
+using ConstPointerIteratorTraits = std::iterator_traits<const int*>;
+static_assert(std::same_as<ConstPointerIteratorTraits::iterator_category,
+                           std::random_access_iterator_tag>);
+static_assert(std::same_as<ConstPointerIteratorTraits::value_type, int>);
+static_assert(
+    std::same_as<ConstPointerIteratorTraits::difference_type, std::ptrdiff_t>);
+static_assert(std::same_as<ConstPointerIteratorTraits::reference, const int&>);
----------------
cjdb wrote:
> cjdb wrote:
> > `libcxx/test/std/iterators/iterator.primitives/iterator.traits/const_pointer.pass.cpp` has 5/6 of these tests. Please move the `iterator_concept` case there.
> > 
> > `libcxx/test/std/iterators/iterator.primitives/iterator.traits/volatile_pointer.pass.cpp` and `libcxx/test/std/iterators/iterator.primitives/iterator.traits/const_volatile_pointer.pass.cpp` will need updates too.
> Ping.
I think this is done, no? I just haven't removed the tests from here as well. 


================
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>;
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855