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

Fri Apr 29 01:47:50 PDT 2022

philnik added inline comments.

================
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)
----------------
philnik wrote:
> devin.jeanpierre wrote:
> > philnik wrote:
> > > Do we want this function in the dylib?
> > Unfortunately I'm not sure what this is asking, and probably don't have the background to give an informed answer. Anyone else want to step in here?
> I think that's a question @ldionne has to answer. My question is essentially if we want to have this as part of the ABI or if it should be marked `_LIBCPP_HIDE_FROM_ABI`.
We are in vector so this won't be exported in our dylib. I think this should be marked `_LIBCPP_HIDE_FROM_ABI`.

================
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:
> > > > 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.

================
Comment at: libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp:116-131
+static_assert(!std::__libcpp_is_trivially_relocatable<std::deque<Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::forward_list<Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::list<Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::map<Nontrivial, Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::multimap<Nontrivial, Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::multiset<Nontrivial>>::value, "");
+static_assert(!std::__libcpp_is_trivially_relocatable<std::set<Nontrivial>>::value, "");
----------------
devin.jeanpierre wrote:
> philnik wrote:
> > Do you have any interest in making a follow-up PR for these? It should be possible to make these all `trivially_relocatable`, right?
> Yes, I think we should put work into making many common datatypes [[clang::trivial_abi]], in the same way as already done for unique_ptr, and I want to do this for at least a couple common/easy ones (hoping std::vector, std::string qualify).
> 
> Others might require more substantial design work to make this happen (e.g. the map types), and it might be that yet others fundamentally can't (e.g. std::list -- something in there is going to be inherently non-trivially-relocatable, since it's self-referential by design).
Great! I think starting with `string` would be good. That's like the most commonly used type in the library and there won't be that many more changes in the near future AFAIK.

================
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
+
----------------
This shouldn't be here.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385