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

Tue May 3 09:26:13 PDT 2022

philnik accepted this revision as: philnik.
philnik added a comment.

>From me it's just nits at this point, so LGTM. Please wait for @ldionne to approve.

================
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.
----------------
Why does this comment exist?

================
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++.
----------------
`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.

================
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;
----------------
Could you reformulate this to make it a TODO?

================
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 &&
----------------
I think this should be equivalent.

================
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
+> {};
----------------

================
Comment at: libcxx/include/vector:337-342
+template<class _Tp, class _Allocator>
+struct __vector_relocate_trivial_move : integral_constant<bool,
+    __vector_trivial_relocate<_Tp, _Allocator>::value ||
+    (is_trivially_move_constructible<_Tp>::value &&
+    __has_default_allocator_construct<_Allocator, _Tp, _Tp&&>::value)
+> {};
----------------

================
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);
----------------

================
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> {};
+
----------------
devin.jeanpierre wrote:
> philnik wrote:
> > devin.jeanpierre wrote:
> > > philnik wrote:
> > > > devin.jeanpierre wrote:
> > > > > philnik wrote:
> > > > > > 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.
> > > > > > 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.
> > > > > 
> > > > > Agreed, and done!
> > > > > 
> > > > > > 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.
> > > > > 
> > > > > Hmmm, correct me if I'm wrong, but I think it's the `#else` block that doesn't do anything. These two lines are basically equivalent:
> > > > > 
> > > > > ```
> > > > > struct __is_nothrow_constructible_a : integral_constant<bool, false> {};
> > > > > struct __is_nothrow_constructible_a : integral_constant<bool, noexcept(allocator_traits<_Allocator>::construct(declval<_Allocator&>(), (_Tp*)nullptr, declval<_Up>()))> {};
> > > > > ```
> > > > > 
> > > > > ... because `allocator_traits<T>::construct` is never noexcept unless you specialize it, which libc++ does not permit.
> > > > > 
> > > > > The intent of the check is clearly that the `::construct` call will not throw, which we can determine, instead, by if the allocator defines construct at all. (Since we don't support overloading the trait).
> > > > > 
> > > > > Something like: `is_nothrow_constructible<_Tp, _Up>::value && (__is_default_allocator<_Allocator>::value || !__has_construct<_Allocator, _Tp*, _Tp>::value`
> > > > > 
> > > > > Does that look good?
> > > > It definitely makes sense to me. Maybe you could check if the `allocator::construct` member function is `noexcept`. That would include more allocators. Something like `!__has_construct<_Allocator, _Tp*, _Tp>::value || __nothrow_invocable<_Allocator::construct, _Tp*, _Tp>::value` should work. (I haven't tested it.)
> > > It's more complicated than that, because the allocator is allowed not to define construct -- so we need to do the dance that __has_construct does to ensure that this fails gracefully (SFINAE) when it doesn't exist.
> > > 
> > > Can we defer this / not implement it now?  I played around with this a bit, but as it turns out I'm really not very familiar with template metaprogramming in C++. If this is a strongly desired feature, I can write a followup as my very next change, but otherwise I'd actually prefer not to write it at all until a concrete use presents itself. :)
> > > 
> > > (I think such concrete uses may not exist at all, or be very rare -- it'd be a construct function that is defined only for types with noexcept constructors, which is I think impossible to guarantee in C++ (you can't enumerate all the constructors). So the only allocators that would satisfy this are those that are special-cased to known types to be noexcept, or which are noexcept even though the underlying constructors might throw.)
> > You could have something like `void construct(Args&&... args) noexcept(noexcept(T(args...)))`. I don't think that's too rare, but we can do that later if you want. If you don't feel comfortable writing that fun stuff I can try as well.
> Ah, that goes to show my unfamiliarity with exceptions/noexcept. That's a compelling example, and I agree we should support it.
> 
> I think this would make a very decent standalone PR after the fact -- I don't think we need to do it in this PR. I will, uh, iron out the implementation with some coworkers who know the idioms here, and send a followup PR. So I'm marking as resolved for now. Should I add some form of TODO comment to mark this as a potential improvement, or no?
I think adding a TODO would be good, since we know it can be improved relatively easily.

================
Comment at: libcxx/test/libcxx/containers/sequences/vector/insert_trivially_relocatable.pass.cpp:13
+
+// UNSUPPORTED: c++03
+
----------------
Why exactly is it not supported in C++03?

================
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
+
----------------
Missed one.

================
Comment at: libcxx/test/libcxx/utilities/meta/meta.unary.prop/is_trivially_relocatable.compile.pass.cpp:12
+// __libcpp_is_trivially_relocatable
+// __libcpp_is_trivially_relocatable_v
+
----------------
devin.jeanpierre wrote:
> philnik wrote:
> > This shouldn't be here.
> Definitely `__libcpp_is_trivially_relocatable_v` should be deleted, but I think `__libcpp_is_trivially_relocatable` does belong here -- it emulates the style of the other files. (Except it should start with `// <type_traits>`, not `// type_traits`).
> 
> For example, `variant_size.pass.cpp` opens with:
> 
> ```
> // <variant>
> 
> // template <class ...Types> class variant;
> ```
> 
> or, `tuple_array_template_depth.pass.cpp` opens with:
> 
> ```
> // <tuple>
> 
> // template <class... Types> class tuple;
> 
> // template <class Tuple, __tuple_assignable<Tuple, tuple> >
> //   tuple & operator=(Tuple &&);
> 
> // This test checks that we do not evaluate __make_tuple_types
> // on the array when it doesn't match the size of the tuple.
> ```
> 
> I see a similar pattern in every test file I open here: they (usually) list the header they're testing, and (always) list the specific function or type being tested.
> 
> (They do it below the UNSUPPORTED line though, so moved that.)
I only meant the `__libcpp_is_trivially_relocatable_v` :)

================
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) {
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385