[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/include/iterator:735
+template<__specifies_members _Ip>
+struct __iterator_traits<_Ip> {
+  using iterator_category  = typename _Ip::iterator_category;
----------------
zoecarver wrote:
> zoecarver wrote:
> > Can you define this //before// `iterator_traits`? I feel like there's needless complexity here. 
> This is a sort of general note for the whole patch. I think it's best to define things and then use them. I don't really see any benefit to declaring them, using them, then defining them. And it makes it harder to read/understand, increases code size, complexity, compile time, etc. 
Big +1 to what @zoecarver said. More code = more bugs; but even more importantly, complicated code = complicated bugs. Let's make this code as simple as possible. That means as few forward declarations as possible: instead, define entities in the "logical order" so that we build up linearly from the simple details to the big-picture top-level users, with as little "mixing" of levels as possible.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:82
+template <class I, class Wrapper>
+struct get_reference {
+  using type = std::iter_reference_t<Wrapper>;
----------------
zoecarver wrote:
> cjdb wrote:
> > zoecarver wrote:
> > > I feel like both this and `get_pointer` are doing the thing where you write the same implementation twice and test that they do the same thing. IMHO that's not a great way to write tests. 
> > This was the best I could come up with. If you're able to think of a different way to get this information, I'm all ears.
> I think thinking about it as "getting this information" is not the right way to think about it in the context of a test. The test's job isn't to get that information. It's to verify that the implementation knows how to get that information. 
> 
> This will require writing this test a bit differently, so maybe before implementing my suggestion (if you're amenable to it), we should check with others (cc @ldionne and @Quuxplusone). 
> 
> What if you wrote `check_wrapper_traits` as
> 
> ```
> template <class I, class Wrapper, class Category, class Pointer, class Reference>
> constexpr bool check_wrapper_traits() {
>   // ...
>   static_assert(std::same_as<typename Traits::reference, Reference>);
>   static_assert(std::same_as<typename Traits::pointer, Pointer>);
> }
> ```
> In this example, you'd have to launder `Pointer` and `Reference` through the other helper templates, so maybe it's not ideal, but I hope you get what I'm saying (if not, I'll explain more clearly). 
> 
> Basically, what I'm suggesting is that you give the test the literal expect value that you (as the human) figure out by looking at the standard. Just like what's done in `check_with_legacy_iterator`. 
@zoecarver: I spent a while and wrote up how //I'd// test all the codepaths here — by following the case-by-case enumeration laid out [on cppreference](https://en.cppreference.com/w/cpp/iterator/iterator_traits). Here it is:

https://godbolt.org/z/qqKeM3do9

Basically, we expect a special case for pointers; and then we have the simple cases for "all five user-provided" and "four of the five user-provided"; and then we get into all the LegacyFooIterator concepts; and finally we have the fallback to LegacyIterator a.k.a. output-iterator.  All of these tests pass with libstdc++ and with MSVC, so I'd hope they pass with libc++'s implementation as well.

I didn't get around to writing the tests for `static_assert(isnt_an_iterator_v<T>)`. I'd probably put those tests in a separate .cpp file, because they'd look very different.


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/legacy_iterator_wrappers.h:108-110
+  decltype(auto) operator[](difference_type const n) const {
+    return *(*this + n);
+  }
----------------
This function shouldn't need a body, any more than the other member functions do.
(It's also weird that you're building up these "layered" overload sets of member and friend functions; e.g. a `legacy_random_access_iterator` will be implicitly convertible to a `legacy_iterator` whose `operator++` has a different return type from the original. Personally I wouldn't use inheritance for this kind of thing; I'd do what "test_iterators.h" does and write completely minimal "archetypes" with no metaprogramming at all. Any chance we could convince you to pursue that minimal direction?)

(EDIT: I've done this now. Here's my suggested tests: https://godbolt.org/z/qqKeM3do9 )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855