[libcxx-commits] [PATCH] D118400: [libc++] Remove operator-> from iterator archetypes that don't need it

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 2 inline comments as done.
ldionne added inline comments.


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/arrow.pass.cpp:24-26
   // Case 1: http://eel.is/c++draft/iterators.common#common.iter.access-5.1
   {
+    auto check = []<class Iterator>() {
----------------
Quuxplusone wrote:
> Nit: Why not good old-fashioned
> ```
> template<class It>
> void test_case_1() {
>     // Case 1: http://eel.is/c++draft/iterators.common#common.iter.access-5.1
>     ~~~
> }
> 
> template<class It>
> void test_case_2() {
> ```
> etc? The whole `check.operator()<T>();` thing //works//, but it doesn't //look nice//.
I actually applied this suggestion, and it resulted in the test case definition and the types it is being called on being further apart, which seemed to decrease legibility. So I'd be in favour of keeping the current approach (even though I agree `check.operator()<T>()` doesn't look pretty).


================
Comment at: libcxx/test/std/iterators/predef.iterators/iterators.common/iterator_traits.compile.pass.cpp:94
     static_assert(std::same_as<IterTraits::difference_type, std::ptrdiff_t>);
-    static_assert(std::same_as<IterTraits::pointer, const Iter&>);
+    static_assert(std::same_as<IterTraits::pointer, int*>);
     static_assert(std::same_as<IterTraits::reference, int&>);
----------------
Quuxplusone wrote:
> Quuxplusone wrote:
> > The code on the left strikes me as weird. But whatever, I'm getting deja vu to a conversation with @CaseyCarter where he said nobody cares about the value of `::pointer`. :)
> For my information, did we ever figure out why the old code thought it'd be `const Iter&`?
> ("No" is an acceptable answer. ;))
Frankly, I did not investigate because `int*` is what I would have expected all along. However, thinking about it right now:

http://eel.is/c++draft/iterators.common#common.iter.access-5.1 says that we return `return get<I>(v_­);`, when the iterator has `operator->`, and that is `Iter const&` here. When we remove `operator->` from the underlying iterator, we instead return `addressof(*get<I>(v_))`, and that is `int*`, as we're seeing in the diff after.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118400