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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: include/__split_buffer:279
     while (__begin_ != __new_begin)
-        __alloc_traits::destroy(__alloc(), __to_raw_pointer(__begin_++));
+        __alloc_traits::destroy(__alloc(), _VSTD::__to_raw_pointer(__begin_++));
 }
----------------
The changes adding `_VSTD::` LGTM.


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




================
Comment at: include/memory:1349
         is_same<
-            decltype(__has_construct_test(declval<_Alloc>(),
-                                          declval<_Pointer>(),
-                                          declval<_Args>()...)),
+            decltype(_VSTD::__has_construct_test(declval<_Alloc>(),
+                                                 declval<_Pointer>(),
----------------
Wouldn't the `declval` calls also need qualification?


https://reviews.llvm.org/D37538