[libcxx-commits] [PATCH] D119385: Use trivial relocation operations in std::vector, by porting D67524 and part of D61761 to work on top of the changes in D114732.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks for the patch! I have some comments and questions.

Also, I'd like to understand the relationship between this patch and Arthur's series of patches for trivially relocatable types -- is the intent that this one replaces the series (even though it's a slimmer version of Arthur's patches IIUC)?



================
Comment at: libcxx/include/__memory/allocator_traits.h:401
+template <class _Alloc, class _Tp, class... _Args>
+struct __has_trivial_construct : integral_constant<bool, (__is_default_allocator<_Alloc>::value ||
+                                                          !__has_construct<_Alloc, _Tp*, _Args...>::value) > {};
----------------
I find this misleading. Indeed, the construction of `_Tp` might not be trivial at all, it's just that the allocator doesn't do something fancy in addition to constructing the object. In other words, I think something like `__allocator_construct_is_placement_new` is what we *mean*, but that name sucks. Other potential names: `__has_default_allocator_construct`, `__overrides_allocator_construct`, `__has_custom_allocator_construct`.

In particular, I would avoid mentioning "trivial" anywhere because it makes it look as though the operation itself is trivial, which is not what we mean.

The same comment applies to `__has_trivial_destroy` below.


================
Comment at: libcxx/include/memory:852
 _LIBCPP_INLINE_VISIBILITY
-void __construct_forward_with_exception_guarantees(_Alloc& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __begin2) {
+void __construct_forward_with_exception_guarantees(_Alloc& __a, _Ptr __begin1, _Ptr __end1, _Ptr& __begin2, false_type) {
     static_assert(__is_cpp17_move_insertable<_Alloc>::value,
----------------
I find those out-of-the-blue `false_type` and `true_type`s awkward. Instead, I would much rather make the decision of which overload to take based on `enable_if` right here, in these functions. That way, `std::vector` would just call `__construct_forward_with_exception_guarantees(a, b, e, b2)` without having to pass `__move_via_memcpy()`.


================
Comment at: libcxx/include/type_traits:357-358
         = is_trivially_destructible<T>::value;                           // C++17
+      template <class T> inline constexpr bool __libcpp_is_trivially_relocatable_v
+        = __libcpp_is_trivially_relocatable<T>::value;                   // extension
       template <class T, class... Args> inline constexpr bool is_nothrow_constructible_v
----------------
I'm fine if we make optimizations inside the library, but I don't want us to expose a new "customization point" that hasn't been standardized. That's just going to be a pain in the future, and it's a portability trap for our users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385