[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 May 5 06:14:25 PDT 2022

devin.jeanpierre added a comment.

A couple notes:

- I was really struggling to run tests locally and only uploaded this to run the tests. Unfortunately, that also did things like mark comments as resolved, oops. Tomorrow I might try again to get local tests working, as I expect many rounds of test fixing, and CI is taking longer than expected.
- I struggled a bit with git and lost some work. Did a couple passes over everything to make sure I actually addressed everything I hit done on (and didn't e.g. fix it before accidentally losing work), but I'm very sorry if I missed something!

Aside from comments, also tried to fix test failures (e.g. fixed my local copy of clang-format so that it runs -- my copy of Debian doesn't have the `python` binary that `git clang-format` expects, just `python2` and `python3`; and disabled execution of the exception test on tests that disable exceptions)

================
Comment at: libcxx/include/memory:856-857

+// __construct_forward_with_exception_guarantees(__a, __begin1, __end1, __begin2, __use_memcpy)
+//
+// If __use_memcpy is true_type, this constructs using memcpy. Otherwise, it uses the move constructor.
----------------
philnik wrote:
> Why does this comment exist?
Without reading the implementation, one doesn't know the difference between `__construct_forward_with_exception_guarantees(a, x, y, z, true_type)` and `__construct_forward_with_exception_guarantees(a, x, y, z, false_type)`, so I figured documentation was appropriate to clarify. (Well, not that this function is incredibly well-documented right now.)

I can delete it if it's actually obvious and these overloads mean the same thing everywhere.

================
Comment at: libcxx/include/memory:880
+void __construct_forward_with_exception_guarantees(_Alloc&, _Ptr __begin1, _Ptr __end1, _Ptr& __begin2, true_type) {
+    typedef typename iterator_traits<_Ptr>::value_type _Tp;
+    // Note: const_cast used to support const value types until support is removed from libc++.
----------------
philnik wrote:
> `using` is supported as an extension in C++03. You don't have to use `using`, but I think it's a lot nicer to read.
Ah, sorry. I should've gone through the whole PR and fixed these the first time around. Done.

The other instance I replaced was in __construct_backward_with_exception_guarantees.

================
Comment at: libcxx/include/memory:881
+    typedef typename iterator_traits<_Ptr>::value_type _Tp;
+    // Note: const_cast used to support const value types until support is removed from libc++.
+    typedef typename remove_const<_Tp>::type _Vp;
----------------
philnik wrote:
> Could you reformulate this to make it a TODO?
Done!

================
Comment at: libcxx/include/type_traits:3108-3117
+template <class _Tp>
+struct _LIBCPP_TEMPLATE_VIS __libcpp_is_trivially_relocatable
+    : public integral_constant<bool,
+#if __has_keyword(__is_trivially_relocatable)
+    __is_trivially_relocatable(_Tp)
+#else
+    is_trivially_move_constructible<typename remove_all_extents<_Tp>::type>::value &&
----------------
philnik wrote:
> I think this should be equivalent.
Also gets rid of the memey #if inside of the integral_constant type.

================
Comment at: libcxx/include/vector:325-330
+template<class _Tp, class _Allocator>
+struct __vector_trivial_relocate : integral_constant<bool,
+    __libcpp_is_trivially_relocatable<_Tp>::value &&
+    __has_default_allocator_construct<_Allocator, _Tp, _Tp&&>::value &&
+    __has_default_allocator_destroy<_Allocator, _Tp>::value
+> {};
----------------
philnik wrote:
> 
This looks prettier than it has any right to. :)  Done, and thank you!

================
Comment at: libcxx/include/vector:718-720
+    template<class _Up>
+        _LIBCPP_HIDE_FROM_ABI
+        void __relocate_upward_and_insert(pointer __from_s, pointer __from_e, _Up&& __elt);
----------------
philnik wrote:
> 
Apologies, I had hoped arc diff would autoformat this correctly.

================
Comment at: libcxx/test/libcxx/containers/sequences/vector/insert_trivially_relocatable.pass.cpp:13
+
+// UNSUPPORTED: c++03
+
----------------
philnik wrote:
> Why exactly is it not supported in C++03?
Ah. I had copied this from the change I'm forward porting, but I believe it is trivial to support C++03 in this test, so I've done so and removed the comment.

================
Comment at: libcxx/test/libcxx/utilities/meta/meta.unary.prop/is_trivially_relocatable.compile.pass.cpp:14
+// __libcpp_is_trivially_relocatable
+// __libcpp_is_trivially_relocatable_v
+
----------------
philnik wrote:
> Missed one.
Sorry, I don't know what happened there.

================
Comment at: libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_exception_safety.pass.cpp:64
+void test() {
+  using T = MyClass<!!(Mask & 1), !!(Mask & 2), !!(Mask & 4), !!(Mask & 8), !!(Mask & 16)>;
+  for (int n = 0; true; ++n) {
----------------
philnik wrote:
> devin.jeanpierre wrote:
> > philnik wrote:
> > > What does that do? If it's just a cast to `bool` could you maybe just make if a functional cast? That's a lot less surprising.
> > Not sure what you mean by functional cast, but maybe it'd be clearer as a `;;`?
> > 
> > I also added a comment to document the test itself and its behavior.
> I mean that you could use `bool(Mask & 1), ...`. I think it's actually called function-style cast, not functional cast.
Sorry, I think phabricator was attributing your comment to the wrong line (I saw it on the for statement before. At the moment, it's now centered on a comment line.). Replaced with casts.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385