[PATCH] D48342: [libcxx] Optimize vectors construction of trivial types from an iterator range with const-ness mismatch.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.


================
Comment at: libcxx/include/memory:1479
+struct __has_construct_missing
+    : false_type
+{
----------------
Quuxplusone wrote:
> vsapsai wrote:
> > vsapsai wrote:
> > > vsapsai wrote:
> > > > erik.pilkington wrote:
> > > > > vsapsai wrote:
> > > > > > erik.pilkington wrote:
> > > > > > > Shouldn't this be true_type?
> > > > > > I see this is confusing and I'm still struggling how to express it. The issue is that in C++03 `__has_construct` should be something like unknown, so that neither `__has_construct` nor `! __has_construct` evaluate to true because we don't really know if allocator has construct. This case is covered by the added test, in C++03 the memcpy specialization was enabled when
> > > > > > 
> > > > > > ```
> > > > > >     is_same<allocator_type, allocator<_Tp> >
> > > > > >       || !false_type
> > > > > > ```
> > > > > > 
> > > > > > So `is_same` check had no effect and we were using memcpy to convert between int and float.
> > > > > > 
> > > > > > I was considering using something like
> > > > > > 
> > > > > > ```lang=c++
> > > > > >         typename enable_if
> > > > > >         <
> > > > > >             (is_same
> > > > > >              <
> > > > > >                 typename _VSTD::remove_const<typename allocator_type::value_type>::type,
> > > > > >                 typename _VSTD::remove_const<_SourceTp>::type
> > > > > >              >::value
> > > > > > #ifndef _LIBCPP_CXX03_LANG
> > > > > >                 || !__has_construct<allocator_type, _DestTp*, _SourceTp>::value
> > > > > > #endif
> > > > > >             ) &&
> > > > > >              is_trivially_move_constructible<_DestTp>::value,
> > > > > >             void
> > > > > >         >::type
> > > > > > ```
> > > > > > 
> > > > > > But that doesn't look readable to me, so I've introduced ad-hoc ternary logic with `__has_construct_missing`.
> > > > > Oh I see, yikes! That's a pretty bad bug. I agree that this fix is best then, but can you add a comment explaining this to `__has_construct_missing` for future casual readers? Also, I think we should move the __has_construct_missing bugfix into a different (prerequisite) patch. Seems unrelated to the `const` related optimization below.
> > > > The bug as I described isn't really present now because function signature
> > > > 
> > > >     __construct_range_forward(allocator_type&, _Tp* __begin1, _Tp* __end1, _Tp*& __begin2)
> > > > 
> > > > works as implicit `is_same` for `__begin1` and `__begin2` types. I think it is worth fixing separately and there is a bug with C++03 and custom allocators.
> > > Instead of `__has_construct_missing` I've implemented real `__has_construct` in D48753. But it is stricter in C++03 than in C++11 and later. So it made me think that absence of `construct` with exact signature isn't a good reason to use memcpy.
> > I was wrong. Now I think the logic for using memcpy should be
> > 
> >     if types are the same modulo constness
> >     and
> >     (
> >         using default allocator
> >         or
> >         using custom allocator without `construct`
> >     )
> >     and
> >     is_trivially_move_constructible
> > 
> > The purpose of the allocator check is to cover cases when `static construct` would end up calling not user's code but libc++ code that we know can be replaced with memcpy.
> I'd like to request the spelling `__has_trivial_construct_and_destroy<A, T, T&&>` as described here: https://www.youtube.com/watch?v=MWBfmmg8-Yo&t=21m45s
> I have an implementation in my fork that might be useful for comparison, although even it doesn't add that last `T&&` parameter.
> https://github.com/Quuxplusone/libcxx/commit/34eb0b5c8f03880b94d53b877cbca384783ad65a
> 
> What we're *really* interested in, in most cases, is `__has_trivial_construct<A, T, T&&> && __has_trivial_destroy<A, T>`. We don't care if there happens to exist an obscure overload such as `A::construct(T*, Widget, Gadget)` as long as it is not selected. This is particularly relevant to `scoped_allocator_adaptor`, whose `construct` is trivial for primitive types but non-trivial for allocator-aware types.
> 
> Also, when we write out the template type parameters we care about, we can do the decltype stuff really easily, without having to "fudge" the metaprogramming and possibly get it wrong. For example, if `A::construct` is an overload set, in which case the `&_Xp::construct` on this patch's line 1492 will be ill-formed even though a non-trivial `construct` does actually exist.
I agree with benefits of using `__has_trivial_construct_and_destroy` in general but in this case I don't see a need for `__has_trivial_destroy`. `__construct_range_forward` only copies new elements and it isn't concerned with destruction. Probably for some cases we can meld destroy+construct together but I think that is out of scope for the current patch.

Arthur, can you please give some examples of possible problems with `scoped_allocator_adaptor`. I didn't work with it closely and don't really understand your concern.

I wish I could avoid "fudging" the metaprogramming  but we need to support C++03 and I don't see other alternatives. And in C++03 `A::construct` shouldn't be an overload set so my understanding is that it is OK to use `&_Xp::construct`.


https://reviews.llvm.org/D48342