[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:72
+  struct reference {};
+  // TODO
+  value_type*
----------------
cjdb wrote:
> TODO?
To remind me to fix the pointer thing. No longer needed. Thanks. 


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:278
+
+struct LegacyRandomAccess {
+  struct value_type {};
----------------
cjdb wrote:
> I think these are a duplication of what's in `test/libcxx/iterators/iterator.requirements/iterator.assoc.types/iterator.traits/iterator_traits_cpp17_iterators.h`. Could you please confirm?
Yes, they're very similar. I think the only real difference is the difference_type. This one has a weird difference type, which I think is good because it shows that it's getting it from the correct place. 

That being said, this is a fairly small object, and I think it's beneficial to the reader to have the short test next to the short object. It makes it easy to see exactly what the input and output it without having to look for something or think too hard. But that's just my opinion, maybe others feel differently. 


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp:103
     }
-
+#endif
     {
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855