[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:82
+template <class I, class Wrapper>
+struct get_reference {
+  using type = std::iter_reference_t<Wrapper>;
----------------
Quuxplusone wrote:
> 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.
I like these tests //a lot//. I think this is a perfect example of how we should be testing these things (IMHO). All the cases are covered, and yet, they are super simple and easily verifiable. When I look at something like:

```
using PT = std::iterator_traits<int*>;
static_assert(std::same_as<PT::iterator_category, std::random_access_iterator_tag>);
static_assert(std::same_as<PT::value_type, int>);
static_assert(std::same_as<PT::difference_type, std::ptrdiff_t>);
static_assert(std::same_as<PT::reference, int&>);
static_assert(std::same_as<PT::pointer, int*>);
static_assert(std::same_as<PT::iterator_concept, std::contiguous_iterator_tag>);
```

it is immediately clear a) what is being tested, and b) if the test is correct. If you have something computed, like:

```
  static_assert(std::same_as<typename Traits::reference,
                             typename get_reference<I, Wrapper>::type>);
```

It isn't as clear what iterator is being tested, but more importantly, it isn't clear that the test is correct. I have to go through all the logic of who is calling this function, what the I and Wrapper types are, and then what the implementation logic for `get_reference` is. All of those are error prone and not really necessary. For tests we shouldn't be testing that the implementation does the same thing as X, we should be testing that the implementation does the right thing. 

I would be in support of using those tests instead (or maybe in addition to some of the existing tests). Please update some of the iterator names, though (such as `A` and `B` to be more descriptive). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855