[libcxx-commits] [PATCH] D101638: [libc++] std::to_address mustn't depend on P::element_type.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added a comment.

Thanks for fixing this. This LGTM, but we need to figure out how to write the tests correctly.



================
Comment at: libcxx/test/std/utilities/memory/pointer.conversion/to_address_std_iterators.pass.cpp:32
+    assert(std::to_address(c.begin()) == c.data());
+    assert(std::to_address(c.end()) == c.data() + c.size());
+    assert(std::to_address(cc.begin()) == cc.data());
----------------
zoecarver wrote:
> Quuxplusone wrote:
> > zoecarver wrote:
> > > zoecarver wrote:
> > > > @Quuxplusone this is the reason the test is failing in debug mode, we can't dereference end. Do we ever really want to be dereferencing end anyway, isn't that UB? I don't think we need to test this, at least not in debug mode. 
> > > Actually on second thought: we're not actually dereferencing anything here. I guess when the assertion was added to `operator->` they didn't expect the caller of the operator to be using the pointer itself. So I feel like there's a good argument to remove the assertion all together. @EricWF, thoughts?
> > FYI @zoecarver, I believe you've got it now. But I don't think "remove the assertion" is correct. I think, (well, as you know I think) `pointer_traits` was just a dumb place to put this customization point (it should have been ADL like `swap`), but, I think the whole intention of //letting// the user customize `to_address` was //precisely// to give them a way to solve this exact problem. They have an `operator->` that is unsafe to call on non-dereferenceable pointers, so they should customize `to_address`.
> > 
> > "They/the user" here means `__wrap_iter`, of course, which means us.
> > 
> > IOW, the answer is clearly to have libc++ provide a partial specialization of `pointer_traits<__wrap_iter<U>>` and maintain it forever. That's clearly the right thing to do. I resist it only because it's so, so dumb.
> > FYI @zoecarver, I believe you've got it now. But I don't think "remove the assertion" is correct. I think, (well, as you know I think) pointer_traits was just a dumb place to put this customization point (it should have been ADL like swap), but, I think the whole intention of letting the user customize to_address was precisely to give them a way to solve this exact problem. They have an operator-> that is unsafe to call on non-dereferenceable pointers, so they should customize to_address.
> >
> > "They/the user" here means __wrap_iter, of course, which means us.
> >
> > IOW, the answer is clearly to have libc++ provide a partial specialization of pointer_traits<__wrap_iter<U>> and maintain it forever. That's clearly the right thing to do. I resist it only because it's so, so dumb.
> 
> Okay... I think you're right. Even though, as you said, this is super dumb. 
> 
> However, we should still take this change, because it does fix something that was broken. We just also need to keep the specialization of `to_address`. I think we also need to add an `element_type` to the specialization of `pointer_traits`, otherwise it's ill formed. 
It's actually not clear to me that this test is correct. I don't see why we're allowed to call `to_address` on a one-past-the-end iterator of a container. Imagine another implementation uses a different type of their own to implement e.g. `std::vector<T>::iterator`, then there would be no guarantee that we can use `std::to_address` on that at all, since `vector<T>::iterator` is only required to be a `LegacyRandomAccessIterator` (which doesn't even include `to_address`). Am I mistaken here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101638