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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

I've pinged comments that I think either have been missed or warrant discussion if you disagree with my commentary.



================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:114
+
+static_assert(testStdlibIterator<std::vector<int>::iterator, int, std::random_access_iterator_tag>());
+static_assert(testStdlibIterator<std::string::iterator, char, std::random_access_iterator_tag>());
----------------
Please don't forget `std::vector<bool>::iterator` and `const_iterator`s (I'll turn a blind eye toward all containers' `reverse_iterator` members if `std::reverse_iterator` is tested).


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:124-131
+static_assert(testStdlibIterator<std::set<int>::iterator, int, std::ptrdiff_t, const int&, const int*,
+                                 std::bidirectional_iterator_tag>());
+static_assert(
+    testStdlibIterator<std::map<int, int>::iterator, std::pair<const int, int>, std::bidirectional_iterator_tag>());
+static_assert(
+    testStdlibIterator<std::unordered_map<int, int>::iterator, std::pair<const int, int>, std::forward_iterator_tag>());
+static_assert(testStdlibIterator<std::unordered_set<int>::iterator, int, std::ptrdiff_t, const int&, const int*,
----------------
Please don't forget the multis and local_iterators.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:133-145
+static_assert(testStdlibIterator<std::back_insert_iterator<std::vector<int> >, void, std::output_iterator_tag>());
+static_assert(testStdlibIterator<std::front_insert_iterator<std::vector<int> >, void, std::output_iterator_tag>());
+static_assert(testStdlibIterator<std::insert_iterator<std::vector<int> >, void, std::output_iterator_tag>());
+static_assert(testStdlibIterator<std::istream_iterator<int, char>, int, std::ptrdiff_t, const int&, const int*,
+                                 std::input_iterator_tag>());
+static_assert(
+    testStdlibIterator<std::istreambuf_iterator<char>, char, long long, char, char*, std::input_iterator_tag>());
----------------
Please don't forget `std::reverse_iterator`, the filesystem iterators, and the regex iterators.


================
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:
> `libcxx/test/std/iterators/iterator.primitives/iterator.traits/pointer.pass.cpp` already covers this case.
Ping.


================
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:
> `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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855