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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added inline comments.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:153
+
+struct LegacyInputNoValueType {
+  struct not_value_type {};
----------------
zoecarver wrote:
> 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? 
Renaming to `s/Legacy/IteratorTraitsCpp17/` should do the trick.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp:103
     }
-
+#endif
     {
----------------
zoecarver wrote:
> 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. 
>  Was there another macro you were thinking of?

Specifically `TEST_STD_VER <= 17 || !defined(__cpp_lib_concepts)`. That eliminates the implementation-detail names in userspace altogether :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855