[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 08:04:36 PDT 2022

devin.jeanpierre 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:
> 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`.
Sounds good to me, and thanks for following up here. Done.

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

================
Comment at: libcxx/test/libcxx/utilities/meta/meta.unary.prop/is_trivially_relocatable.compile.pass.cpp:83
+
+#if TEST_STD_VER >= 11
+static_assert(!std::__libcpp_is_trivially_relocatable<NontrivialMoveOnly>::value, "");
----------------
I have a feeling that this test is going to have failures on a few platforms (e.g. PS4, Win64), now that I've had experience with the predecessor change and its rollbacks. I'm asking in discord for how to run the tests locally, without doing a slow upload / look at build results loop. (I haven't even looked at the failures for this right here, actually.)

https://discord.com/channels/636084430946959380/642374147640524831/971015700665761833

Just leaving this (unresolved) comment to mark that additional work is needed here.

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

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385