[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
+{
----------------
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.


https://reviews.llvm.org/D48342