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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added a subscriber: Quuxplusone.
zoecarver added inline comments.


================
Comment at: libcxx/include/iterator:916
+#endif
 struct _LIBCPP_TEMPLATE_VIS iterator_traits<_Tp*>
 {
----------------
cjdb wrote:
> zoecarver wrote:
> > This also needs a few updated: `value_type` should now remove cv, adds `iterator_concept `, adds a const specialization. Are you making those changes in another patch?
> > `value_type` should now remove cv,
> 
> Already applied (see below).
> 
> > adds `iterator_concept`
> 
> Already applied (see below).
> 
> > adds a const specialization
> 
> I don't see this in [iterator.traits]?
> Already applied (see below).

Even better :)

> I don't see this in [iterator.traits]?

Ah, I said the wrong thing. I meant it //removes// the const specialization. And I guess we never had that specialization, so nothing to be done.


================
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>;
----------------
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`. 


================
Comment at: libcxx/test/std/iterators/iterator.primitives/iterator.traits/cxx20_iterator_traits.compile.pass.cpp:289
+};
+static_assert(check_fails<reference_as_static_member>());
----------------
cjdb wrote:
> zoecarver wrote:
> > I have no idea what the consensus here was... are we still doing compile.passp.cpp tests or no? If no, then we should make this a "normal" .pass test.
> Consensus was to keep `.compile.pass.cpp` and for them to no longer have `main`.
Great. I like this idea a lot. Keeping the time it takes to run the tests down :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99855