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

[David L. Jones via Phabricator via cfe-commits]

dlj marked 2 inline comments as done.
dlj 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;
----------------
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.


================
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>(),
----------------
EricWF wrote:
> Quuxplusone wrote:
> > EricWF wrote:
> > > Wouldn't the `declval` calls also need qualification?
> > (Sorry I'm jumping in before I know the full backstory here.) I would expect that the "declval" call doesn't need qualification because its argument-list is empty. ADL doesn't apply to template arguments AFAIK.
> > (Test case proving it to myself: https://wandbox.org/permlink/i0YarAfjOYKOhLFP )
> > 
> > Now, IMHO, `__has_construct_test` also doesn't need guarding against ADL, because it contains a double underscore and is thus firmly in the library's namespace. If the user has declared an ADL-findable entity `my::__has_construct_test`, they're already in undefined behavior land and you don't need to uglify libc++'s code just to coddle such users.
> > 
> > And, IMO, to the extent that ADL *does* work on the old code here, that's a feature, not a bug. As the implementor of some weird fancy pointer or iterator type, I might *like* having the power to hook into libc++'s customization points here. Of course I wouldn't try to hook into `__has_construct_test`, but `__to_raw_pointer` feels more likely. (At my current low level of understanding, that is.)
> >  If the user has declared an ADL-findable entity my::__has_construct_test, they're already in undefined behavior land 
> 
> The problem isn't that it will find a `my::__has_construct_test`, but that the simple act of looking requires completed types, otherwise an error is generated. [Example](https://wandbox.org/permlink/KAgGXkGSjXTETKKw)
> 
>  
Hmm, no I don't think so... declval<_Alloc> is a "declaration of a function at block scope" (because it's a template specialization), so it does not participate in ADL.

I believe the difference with e.g. __has_construct_test is that the other calls name a function template, which is not specialized (and so isn't a function declaration) until it wins the ADL.

I'm a little bit hazy on exactly the semantics, though, so Richard can correct me if I've gotten it substantially wrong. :-)


================
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>(),
----------------
dlj wrote:
> EricWF wrote:
> > Quuxplusone wrote:
> > > EricWF wrote:
> > > > Wouldn't the `declval` calls also need qualification?
> > > (Sorry I'm jumping in before I know the full backstory here.) I would expect that the "declval" call doesn't need qualification because its argument-list is empty. ADL doesn't apply to template arguments AFAIK.
> > > (Test case proving it to myself: https://wandbox.org/permlink/i0YarAfjOYKOhLFP )
> > > 
> > > Now, IMHO, `__has_construct_test` also doesn't need guarding against ADL, because it contains a double underscore and is thus firmly in the library's namespace. If the user has declared an ADL-findable entity `my::__has_construct_test`, they're already in undefined behavior land and you don't need to uglify libc++'s code just to coddle such users.
> > > 
> > > And, IMO, to the extent that ADL *does* work on the old code here, that's a feature, not a bug. As the implementor of some weird fancy pointer or iterator type, I might *like* having the power to hook into libc++'s customization points here. Of course I wouldn't try to hook into `__has_construct_test`, but `__to_raw_pointer` feels more likely. (At my current low level of understanding, that is.)
> > >  If the user has declared an ADL-findable entity my::__has_construct_test, they're already in undefined behavior land 
> > 
> > The problem isn't that it will find a `my::__has_construct_test`, but that the simple act of looking requires completed types, otherwise an error is generated. [Example](https://wandbox.org/permlink/KAgGXkGSjXTETKKw)
> > 
> >  
> Hmm, no I don't think so... declval<_Alloc> is a "declaration of a function at block scope" (because it's a template specialization), so it does not participate in ADL.
> 
> I believe the difference with e.g. __has_construct_test is that the other calls name a function template, which is not specialized (and so isn't a function declaration) until it wins the ADL.
> 
> I'm a little bit hazy on exactly the semantics, though, so Richard can correct me if I've gotten it substantially wrong. :-)
Double-underscore names don't actually change the lookup rules, so (as Eric points out) isn't really related to the issue I'm addressing here.

As for extension points... __to_raw_pointer is essentially a selector for the correct expression syntax to get the pointed-to node, somewhat analogous to how std::pointer_traits selects an "unwrapped" pointee type. (In this case, an "extension point" already exists: you can use a custom allocator with fancy pointers... have a look at Eric's comments in include/deque for a good example of how you might write one.)


https://reviews.llvm.org/D37538