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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
