[libcxx-commits] [PATCH] D67524: P1144 "Trivially relocatable" (3/3): optimize std::vector for trivially relocatable types

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone marked 3 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: include/memory:1614
         void
-        __construct_forward(allocator_type& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __begin2)
+        __construct_forward(allocator_type& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __begin2, false_type)
         {
----------------
zoecarver wrote:
> Maybe leave a comment about what the various overloads mean. 
> 
> Also, maybe add a default value (in case we want to use this outside of `std::vector` in the future).  
Defaulted function arguments are the devil.
I consider it a //benefit// that now it's impossible to call `__construct_forward` without explicitly saying whether you want to use memcpy or not; this is because the call to `__construct_forward` will usually be followed by a call to `__destroy_at_end`, and the final parameter to both calls must agree (or else you might call a constructor without the matching destructor, or vice versa).

Adding a comment is a good idea, but I'm not sure the best place to put it. Incidentally, in a previous revision I'd had something like

    template<class X, class UseMemcpy>
    static void foo(X x, UseMemcpy) { ... }
    template<class X>
    static void foo(X x, false_type) { ... }

where the first overload matches `true_type`; but that was unnecessary, error-prone, and not really any less confusing, so I trashed it.


================
Comment at: include/memory:5760
+template <class _Alloc>
+struct __has_customized_allocator_traits<_Alloc, typename allocator_traits<_Alloc>::__is_uncustomized> : false_type {};
+
----------------
zoecarver wrote:
> Nit: it might be less confusing to just use `::allocator_type`.
This is actually doing SFINAE on the value of `__is_uncustomized` — which in user-defined specializations of `allocator_traits` will either be absent (if they went from scratch) or have the wrong value (if they derived from `allocator_traits<allocator<T>>` to fill in the boilerplate).

The `__has_trivial_construct` part of this patch is a watered-down version of D49317. For now and maybe forever, libc++ wisely doesn't permit users to specialize `allocator_traits`. (See https://quuxplusone.github.io/blog/2018/07/14/traits-classes/ .)
So I guess it would be fine for me to eliminate `__has_customized_allocator_traits` from this patch.


================
Comment at: include/vector:479
+    __has_trivial_construct<_Allocator, _Tp,
+#ifndef _LIBCPP_CXX03_LANG
+        _Tp&&
----------------
zoecarver wrote:
> We don't need to do this anymore! Every compiler that we support supports rvalues (I think).
I think you're right, but then I'm confused by the similar code on the return type of `move_if_noexcept` (in trunk). What does `_Tp&&` mean, in C++03 mode? Is it a synonym for `const _Tp&`? If so, why does `move_if_noexcept` need that ifndef?

```
template <class _Tp>
inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11
#ifndef _LIBCPP_CXX03_LANG
typename conditional
<
    !is_nothrow_move_constructible<_Tp>::value && is_copy_constructible<_Tp>::value,
    const _Tp&,
    _Tp&&
>::type
#else  // _LIBCPP_CXX03_LANG
const _Tp&
#endif
move_if_noexcept(_Tp& __x) _NOEXCEPT
{
    return _VSTD::move(__x);
}
```


Repository:
  rCXX libc++

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67524/new/

https://reviews.llvm.org/D67524