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

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
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());
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101638