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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked an inline comment as done.
zoecarver added inline comments.


================
Comment at: libcxx/include/iterator:810
+// [iterator.traits]/3.2.3
+// If the qualified-id `I::iterator­category` is valid and denotes a type, `iterator­category` names
+// that type.
----------------
cjdb wrote:
> Typo (scan the comments for other typos such as `iteratortraits` too).
Ah, phab can't display `\x{AD}`. Great. (Not that we should really be using this character anyway...)


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:126
+
+struct LegacyInput {
+  struct iterator_category {};
----------------
cjdb wrote:
> Please also add tests for the minimalistic iterator archetypes in `test/libcxx/iterators/iterator.requirements/iterator.assoc.types/iterator.traits/iterator_traits_cpp17_iterators.h`.
I'll test `iterator_traits_cpp17_iterator`, `iterator_traits_cpp17_input_iterator`, `iterator_traits_cpp17_forward_iterator`. I think everything else is accounted for. 


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:153
+
+struct LegacyInputNoValueType {
+  struct not_value_type {};
----------------
cjdb wrote:
> I think we should assert that each of these types actually meet/don't meet the `cpp17-*-iterator` requirements we expect them to. Also, @ldionne asked that the concepts be renamed to `__iterator_traits_detail::cpp17_*_iterator`, so I think we should reflect that update here.
Sorry, I'm not sure I understand exactly what you're asking for here. Could you elaborate? Do you want me to update `LegacyInputNoValueType` and put it in a namespace? 


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

This seems pretty in line with the other tests in this file. I could argue that we don't need this test to begin with, or that we should be testing this another way, but I won't, because I'm not sure it matters too much :)

> In that case, please switch to feature-test macros.

Well, we haven't yet added the `__cpp_lib_ranges` macro. Was there another macro you were thinking of? I think it's fine to just use ` _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_HAS_NO_CONCEPTS)` because those are already used all over the test suite. To be honest, we could probably use `_LIBCPP_HAS_NO_RANGES ` as well. I was just trying to limit the number of libc++-specific macros we use in "non-libc++" tests. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855