[libcxx-commits] [PATCH] D109668: [libc++][test] Fix iterator assertion in span.cons/deduct.pass.cpp

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/containers/views/span.cons/deduct.pass.cpp:50
     ASSERT_SAME_TYPE(S, std::span<int, 3>);
     assert((std::equal(std::begin(arr), std::end(arr), s.begin(), s.end())));
     }
----------------
jloser wrote:
> Quuxplusone wrote:
> > jloser wrote:
> > > Quuxplusone wrote:
> > > > For another, the right assertion here and throughout is actually
> > > > ```
> > > > assert(s.begin() == arr.begin());
> > > > assert(s.end() == arr.end());
> > > > ```
> > > > That is, we should verify that the span gets the right settings for its members, in terms of the original array, //not// just that it's viewing onto //some// array with the same contents as the original.
> > > > As a bonus, this eliminates the test's dependency on `<algorithm>`.
> > > I like that idea, but I don't think that "just works" because of how span's iterators are `__wrap_iter` depending on `_LIBCPP_DEBUG_LEVEL`, so there won't be a `operator==` to make that valid. Example error:
> > > 
> > > ```
> > > error: invalid operands to binary expression ('int *' and 'std::span<int, 3>::iterator' (aka '__wrap_iter<int *>'))
> > >     assert(std::begin(arr) == s.begin());
> > > ```
> > > 
> > > Is there something I'm missing to make this work regardless of whether the span's iterator type is `pointer` or `__wrap_iter<pointer>`?
> > Ah, gross. Well, since this is C++20, we can certainly do
> > ```
> > assert(std::to_address(s.begin()) == arr.begin());
> > assert(std::to_address(s.begin()) == std::to_address(str.begin()));
> > ```
> > etc. I guess this does explain why it wasn't written that way to begin with; we implemented `span` probably long before we had a working `std::to_address`. But these days we could use `std::to_address`, unless @ldionne objects for any reason.
> Seems reasonable. I just switched to using `std::to_address`. I like it slightly better than the `std::equal` status quo -- only "slightly" because of the noise of not being able to just write `assert(s.begin() == arr.begin());` for example.
> I dug into it for a bit, but I'm not sure how to fix it. The problem is from the statement std::to_address(str.end()). What we want to happen is str.end() to return the wrapped iterator and then pick up the std::to_address defined for wrapped iterators (so it calls its base() function). I don't know why this fires the asserts in __iterator/wrap_iter:95

`std::to_address(it)` ends up calling `it.operator->()`, which contains that assertion, right?
I see an overload of `std::__to_address` in `__iterator/wrap_iter.h`, but I bet it's being #included //after// `__memory/pointer_traits.h` so that that overload isn't in scope at the right time. Maybe it would work to replace `__iterator/wrap_iter.h`'s "overload of `__to_address`" with "partial specialization of `pointer_traits`." I could test that tomorrow, if nobody beats me to it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109668