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

Thu Mar 17 10:27:05 PDT 2022

philnik added inline comments.

================
Comment at: libcxx/include/memory:878
     if (_Np > 0) {
-        _VSTD::memcpy(__begin2, __begin1, _Np * sizeof(_Tp));
+        _VSTD::memcpy(const_cast<_Vp*>(_VSTD::__to_address(__begin2)), _VSTD::__to_address(__begin1), _Np * sizeof(_Tp));
         __begin2 += _Np;
----------------
You can replace `_VSTD` with `std` in your changes. Why are you `const_cast`ing now?

================
Comment at: libcxx/include/vector:310
+    __has_default_allocator_construct<_Allocator, _Tp, _Tp&&>::value &&
+    !is_volatile<_Tp>::value
+> {};
----------------
Every standard library agrees that it's not allowed to have a cv-qualified type as the template parameter for a vector (https://godbolt.org/z/8xEnhEbYT). Ditto below.

================
Comment at: libcxx/include/vector:695-696
     void __move_range(pointer __from_s, pointer __from_e, pointer __to);
+    template<class _Up>
+        void __relocate_upward_and_insert(pointer __from_s, pointer __from_e, _Up&& __elt);
     void __move_assign(vector& __c, true_type)
----------------
Do we want this function in the dylib?

================
Comment at: libcxx/include/vector:900-903
+    typedef integral_constant<bool,
+        __vector_move_via_memcpy<_Tp, _Allocator>::value ||
+        __vector_relocate_via_memcpy<_Tp, _Allocator>::value
+    > __move_via_memcpy;
----------------
That looks very weird. Why is `__move_via_memcpy` move via memcpy or relocate via memcpy? It looks like you are using `__vector_move_via_memcpy` only in two places and both do this weird thing. Why not change `__vector_move_via_memcpy`? The naming for the second typedef looks also weird. (And you can use `using` instead)

================
Comment at: libcxx/include/vector:1644-1650
+    if (__vector_relocate_via_memcpy<_Tp, _Allocator>::value) {
+        _Tp *__rawp = _VSTD::__to_address(__p);
+        __alloc_traits::destroy(this->__alloc(), __rawp);
+        --this->__end_;
+        if (__p != this->__end_) {
+            _VSTD::memmove(__rawp, __rawp + 1, (this->__end_ - __p) * sizeof(*__rawp));
+        }
----------------
Is `this->` needed? If not, please remove it.

================
Comment at: libcxx/include/vector:1649
+        if (__p != this->__end_) {
+            _VSTD::memmove(__rawp, __rawp + 1, (this->__end_ - __p) * sizeof(*__rawp));
+        }
----------------
Could you use `std::move` here? It should behave exactly the same and is easier better to read.

================
Comment at: libcxx/include/vector:1684
+struct __is_nothrow_constructible_a : integral_constant<bool,
+    noexcept(allocator_traits<_Allocator>::construct(declval<_Allocator&>(), (_Tp*)nullptr, declval<_Up>()))
+> {};
----------------
`noexcept` isn't available in C++03 AFAIK.

================
Comment at: libcxx/include/vector:1689
+template<class _Tp, class _Up>
+struct __is_nothrow_constructible_a<allocator<_Tp>, _Tp, _Up> : is_nothrow_constructible<_Tp, _Up> {};
+
----------------
Could you rename this to something like `__is_nothrow_constructible_alloc`? `__is_nothrow_constructible_a` sounds a lot like you needed another name and you just slapped `_a` at the end.
Do you only have this to allow better optimizations in C++03? If that is the case I'd say just remove it. If you care about performace you should switch to C++11 or newer. We currently don't even enable the move overloads in C++03 in most (all?) classes.

================
Comment at: libcxx/test/libcxx/containers/sequences/vector/insert_trivially_relocatable.pass.cpp:13
+
+// UNSUPPORTED: c++98, c++03
+
----------------
We don't support C++98. Ditto for the other tests

================
Comment at: libcxx/test/libcxx/containers/sequences/vector/insert_trivially_relocatable.pass.cpp:19
+
+static int assignments = 0;
+
----------------
Could you make this into a local variable? These tests have to work during constant evaluation in the (hopefully near) future.

================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:1-8
+//===----------------------------------------------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is dual licensed under the MIT and the University of Illinois Open
+// Source Licenses. See LICENSE.TXT for details.
+//
----------------
This looks like the wrong license header.

================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:56-72
+struct T {
+  T();
+  T(const T&);
+  ~T();
+};
+struct R {};
+struct SM {
----------------
Could you give these things a more meaningful name?

================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:78-83
+static const bool NotDebug =
+#if _LIBCPP_DEBUG_LEVEL >= 2
+    false;
+#else
+    true;
+#endif
----------------

================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:85-86
+
+// Define this name for convenience.
+#define is_trivially_relocatable __libcpp_is_trivially_relocatable
+
----------------
Please don't define this kind of stuff for convenience.  What if in a new standard there will be `std::is_trivially_relocatable`? That's not that unlikely IMO and then we would have to rewrite this thing just because you felt like adding a macro here. It's not hard to replace the uses, so you can do it now.

================
Comment at: libcxx/test/libcxx/utilities/meta/meta.unary.prop/is_trivially_relocatable.pass.cpp:16
+// UNSUPPORTED: c++98, c++03
+// XFAIL: gcc-4.9
+
----------------
We don't support gcc-4.9.

================
Comment at: libcxx/test/libcxx/utilities/meta/meta.unary.prop/is_trivially_relocatable.pass.cpp:94
+  return 0;
+}
----------------
You can make this file a `.compile.pass.cpp`.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385