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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


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


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:244
+
+  // Providing difference_type does not fully compensate for missing operator-(It, It).
+  friend MissingMinusIterIter operator-(MissingMinusIterIter, int);
----------------
cjdb wrote:
> I think declaring the operator as deleted would be a good idea.
No, the point of this test is that the operator is missing (not in overload resolution at all). Anyway, this way is shorter, which is good.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp:103
     }
-
+#endif
     {
----------------
cjdb wrote:
> zoecarver wrote:
> > cjdb wrote:
> > > zoecarver wrote:
> > > > Nit: `// _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_HAS_NO_CONCEPTS)`
> > > This should be `!defined(_LIBCPP_HAS_NO_RANGES)` now (I didn't get around to fixing it up).
> > Actually, I don't think so. This is in the `std/` directory. If it was in the libc++ directory, I would agree. But ideally these are "generic" standard library tests. They wouldn't rely on any of our implementation details. 
> In that case, please switch to feature-test macros.
I guess the real question is, why is this test here at all? Maybe it should move to a different test file, which `REQUIRES: c++03 || c++11 || c++14 || c++17`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855