[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 {};
----------------
ldionne wrote:
> zoecarver wrote:
> > cjdb wrote:
> > > 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.
> > I don't want to start bike shedding the name, but some of these names get fairly long. I'd really prefer to use "Legacy" especially in the tests. @ldionne wdyt?
> Just to clarify, I asked that the `__cpp17_xxx_iterator` helper concepts be moved to a namespace after we determined that they were not the same as the `Cpp17XXXIterator` concepts in the Standard, so as to avoid making them easy to mistake with those concepts.
> 
> I don't understand why/how that affects the naming of iterators in the tests. As a result, `LegacyXXXIterator` like done below currently seems acceptable to me.
WFM, I just wanted to bring light to it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855