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

[Devin Jeanpierre via Phabricator via libcxx-commits]

devin.jeanpierre created this revision.
devin.jeanpierre added a reviewer: Quuxplusone.
devin.jeanpierre requested review of this revision.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

This change is based on work from @Quuxplusone in D67524 <https://reviews.llvm.org/D67524> and D61761 <https://reviews.llvm.org/D61761>. All credit to them all blame to me, as I may have accidentally made it worse. :)

This change does not include support for `std::swap` (*relatively* low value, and very difficult to do correctly), just vector operations.

Changes from D61761 <https://reviews.llvm.org/D61761> and D67524 <https://reviews.llvm.org/D67524>:

1. (minor) Use __is_default_allocator instead of a new __is_exactly_std_allocator. At least to my eye, these seemed identical.
2. bugfix to erase: it would `memove` N *bytes*, instead of N *items*.
3. The tests now use std::conditional to enable trivial relocatability, as `[[clang::trivial_abi]]` doesn't take a bool parameter.
4. Due to a (new?) warning that breaks the test, I had to add a layer of indirection to the countdown in insert_exception_safety_pass.
5. in is_trivially_relocatable.pass.cpp:
  - I renamed the classes to suit my personal taste (e.g. `NonEmpty->Polymorphic`, as it's a nonempty polymorphic class).
  - I moved the static_asserts directly into main, so that I could see which one failed. This drops the test that is_trivially_relocatable and is_trivially_relocatable_v are identical, but this can be verified by inspection.
  - The test asserted that `const T` was trivially relocatable, even though it was not trivially move-constructible due to the const. I reversed the test, but I'm not 100% sure whether that was the right fix.
6. Finally, of course, some things in the source tree have changed, and I had to figure out where to put things. Not sure if I did it right!

The most important bit I want feedback on: are there any changes to the compiler needed to actually make it defined behavior to memcpy a trivially-relocatable type? I believe the answer is no, but want to double check. I chatted a bit with @rsmith, and the takeaway that I got was that Clang defines the behavior (or at least, doesn't exploit undefined behavior) for all non-polymorphic types. Since trivially relocatable types cannot be polymorphic, this should be a documentation-only change: we explicitly extend implicit-lifetime types to also include trivially-relocatable types (which are, at least for now, all trivial-abi types.)

It is possibly worthwhile to note that vector *already*, before this change, used `memcpy`or `memmove` on types which have nontrivial destructors, and are therefore not implicit-lifetime. So it already had in mind some extension to implicit-lifetime types, though not one which it describes explicitly. This change further extends it to also include all `__is_trivially_relocatable` types.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D119385

Files:
  libcxx/include/__memory/allocator_traits.h
  libcxx/include/memory
  libcxx/include/type_traits
  libcxx/include/vector
  libcxx/test/libcxx/containers/sequences/vector/insert_trivially_relocatable.pass.cpp
  libcxx/test/libcxx/type_traits/is_trivially_relocatable.pass.cpp
  libcxx/test/libcxx/utilities/meta/meta.unary.prop/is_trivially_relocatable.pass.cpp
  libcxx/test/std/containers/sequences/vector/vector.modifiers/insert_exception_safety.pass.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D119385.407331.patch
Type: text/x-patch
Size: 37480 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20220210/2470a149/attachment-0001.bin>