[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())));
     }
----------------
Quuxplusone wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > 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.
> > > `std::to_address(it)` ends up calling `it.operator->()`, which contains that assertion, right?
> > 
> > I haven't dug into it, but that was my hunch when I saw the CI failure, too.
> > 
> > Actually, thinking about it some more, it's not valid to call `std::to_address` on a `std::string::iterator`, because those are not required to be raw pointers (or even fancy pointers). I believe we should instead be checking that `span.data() == source.data()` and `span.size() == source.size()`. That provides us with the same level of coverage for checking that `std::span`'s constructor does its job, but avoids relying on implementation details of libc++'s iterators.
> > thinking about it some more, it's not valid to call std::to_address on a std::string::iterator, because those are not required to be raw pointers (or even fancy pointers).
> 
> They are required (in C++20) to be //contiguous iterators//, which means calling `std::to_address` on them must be valid. In C++20 there is essentially no difference between a "contiguous iterator" and a "fancy pointer." 
> https://en.cppreference.com/w/cpp/iterator/contiguous_iterator — particularly the semantic requirement involving `std::to_address(c)`
> 
> > I believe we should instead be checking that span.data() == source.data() and span.size() == source.size().
> 
> I agree that that would serve the same purpose, for this test's purposes, so sure, let's do that and ship this. But we should still fix our `std::to_address`, orthogonally.
I've taken a stab at this in D110198. I'm kinda relying on buildkite to tell me if it works, though. 😛 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109668