[PATCH] D37538: [libc++] Remove problematic ADL in container implementations.

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: include/deque:1167-1168
     allocator_type& __a = __alloc();
-    for (iterator __i = begin(), __e = end(); __i != __e; ++__i)
-        __alloc_traits::destroy(__a, _VSTD::addressof(*__i));
+    for (iterator __i = begin(), __e = end(); __i.__ptr_ != __e.__ptr_; __i.operator++())
+        __alloc_traits::destroy(__a, _VSTD::addressof(__i.operator*()));
     size() = 0;
----------------
dlj wrote:
> EricWF wrote:
> > rsmith wrote:
> > > The other changes all look like obvious improvements to me. This one is a little more subtle, but if we want types like `deque<Holder<Incomplete> *>` to be destructible, I think we need to do something equivalent to this.
> > That's really yucky! I'm OK with this, but I really don't like it.
> > 
> > Fundamentally this can't work, at least not generically. When the allocator produces a fancy pointer type, the operator lookups need to be performed using ADL.
> > 
> > In this specific case, we control the iterator type, but this isn't always true. for example like in `__split_buffer`, which uses the allocators pointer type as an iterator.
> > 
> > @dlj I updated your test case to demonstrate the fancy-pointer problem: https://gist.github.com/EricWF/b465fc475f55561ba972b6dd87e7e7ea
> > 
> > So I think we have a couple choices:
> > 
> > (1) Do nothing, since we can't universally support the behavior, so we shouldn't attempt to support any form of this behavior.
> > (2) Fix only the non-fancy pointer case (which is what this patch does).
> > (3) Attempt to fix *all* cases, including the fancy-pointer ones. This won't be possible, but perhaps certain containers can tolerate incomplete fancy pointers?
> > 
> > Personally I'm leaning towards (1), since we can't claim to support the use case, at least not universally. If we select (1) we should probably encode the restriction formally in standardese.
> > 
> > 
> So I think there are at least a couple of issues here, but first I want to get clarification on this point:
> 
> > Do nothing, since we can't universally support the behavior, so we shouldn't attempt to support any form of this behavior.
> 
> To me, this sounds like it could be equivalent to the statement "someone might do a bad job of implementing type-erasure for fancy pointers, so we should never attempt to preserve type erasure of any pointers." Now, that statement seems tenuous //at best// (and a straw man on a slippery slope otherwise), so I'm guessing that's not quite what you meant. ;-)
> 
> That said, it does sort of make sense to ignore deque for now, so I'll drop it from the patch; but for other containers, I don't really see the argument.
> 
> As for #3: I'm able to make (most of) your gist pass for everything but deque... calls to _VSTD::__to_raw_pointer are enough to avoid ADL around operators, although a few other changes are needed, too. That probably makes sense to do as a follow-up.
> 
> As for deque in particular, I think there may just be some missing functions on the __deque_iterator, but it probably warrants a closer look before adding to the interface.
> To me, this sounds like it could be equivalent to the statement "someone might do a bad job of implementing type-erasure for fancy pointers, so we should never attempt to preserve type erasure of any pointers." Now, that statement seems tenuous at best (and a straw man on a slippery slope otherwise), so I'm guessing that's not quite what you meant. ;-)

I'm not sure it's even possible to type-erase fancy pointers in a way your suggesting. Fundamentally I think they need to carry around the pointee type. So this problem isn't so easily explained away or made the fault of the user. Or am I missing something?

The library is allowed, and in this case required, to perform operator lookup via ADL. full stop.  That's how the STL is expected to interact with user types. Even after fixing the accidental ADL caused by `__foo` functions, we haven't and can't "fix" the rest. So now the question becomes "How far should libc++ go to avoid legal ADL lookup when it's possible"?

Whatever we decide, we are agreeing to "support" (or not support) a set of behavior, and I have a complex about claiming to "support" bizarre use cases like this. To support something we need to be able to (1) articulate exactly what behavior is supported, and (2) that the behavior won't regress.

I don't think we can provide guarantee (2) since future changes may require the use of ADL lookup in a way we can't avoid, for example in the same way that `__split_buffer` does. So that's a strike against the possibility of claiming "support".

One possibility is "The default constructor and destructor of containers X, Y, Z do not perform ADL lookup (Constructors A, B, C do not support this behavior) ".  Admittedly it was easier than I thought to fix `__hash_table` and `__tree`, but `__split_buffer` is still a problem, which means that both `deque` and `vector` won't work.

Since we can't fix all of the containers, and the ones we can fix may need to break again in the future, I don't think we can arrive at a meaningful statement of support. Therefore, I think a good argument needs to be made as to why it's important to "fix" the particular cases we can.




https://reviews.llvm.org/D37538