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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/libcxx/utilities/meta/meta.unary.prop/is_trivially_relocatable.pass.cpp:90
+  static_assert(std::__libcpp_is_trivially_relocatable<TrivialMoveOnly>::value, "");
+  static_assert(!std::__libcpp_is_trivially_relocatable<const TrivialMoveOnly>::value, "");
+#endif
----------------
devin.jeanpierre wrote:
> devin.jeanpierre wrote:
> > Quuxplusone wrote:
> > > I would say that in a perfect world `const TrivialMoveOnly` ought to be trivially relocatable, because `TrivialMoveOnly` is a trivially relocatable type, and if you're moving-and-destroying it, then you don't really care that the source object was const-qualified.
> > > However, I see that even under p1144r5, `is_relocatable_v<const TrivialMoveOnly>` would be `false`, and I did not intend that `is_trivially_relocatable_v<T> && !is_relocatable_v<T>` should ever be true for any `T`. So I'll have to change this part of p1144. Most library users, then, should probably use `is_trivially_relocatable<remove_cvref_t<T>>` for their purposes.
> > > 
> > > Relevant question for this PR: Should we permit the libc++ extension type `std::vector<const X>` to actually `push_back` and reallocate when `is_trivially_relocatable_v<X>`? In the past that was obviously impossible, but now it's kinda possible.
> > > Relevant question for this PR: Should we permit the libc++ extension type `std::vector<const X>` to actually `push_back` and reallocate when `is_trivially_relocatable_v<X>`? In the past that was obviously impossible, but now it's kinda possible.
> > 
> > My personal opinion is that this is too dangerous: people will rely on it by accident, not even knowing what they're doing (e.g. templated code with levels of indirection), and that would make it significantly harder to remove or change `__is_trivially_relocatable`.
> > 
> > Separately (bit of a cop-out), I think that should be a separate PR: it's a new feature, whereas this PR is "just" an optimization, if that makes sense.
> > 
> > I would say that in a perfect world `const TrivialMoveOnly` ought to be trivially relocatable, because `TrivialMoveOnly` is a trivially relocatable type, and if you're moving-and-destroying it, then you don't really care that the source object was const-qualified.
> 
> Yeah, it is a bit unfortunate. FWIW I think the correct behavior right now is that it should not be trivially relocatable, as it is not trivially move constructible. If we want to make it considered trivially relocatable, we should also suitably alter the definition of triviality elsewhere -- I think, rather than searching for eligible constructors, the definition would search for eligible constructors without const qualification.
> 
> (No idea for volatile, I try not to think about it.)
> 
> > Relevant question for this PR: Should we permit the libc++ extension type `std::vector<const X>` to actually `push_back` and reallocate when `is_trivially_relocatable_v<X>`? In the past that was obviously impossible, but now it's kinda possible.
> 
> My personal opinion is that this is too dangerous: people will rely on it by accident, not even knowing what they're doing (e.g. templated code with levels of indirection), and that would make it significantly harder to remove or change `__is_trivially_relocatable`.
> 
> Separately (bit of a cop-out), I think that should be a separate PR: it's a new feature, whereas this PR is "just" an optimization, if that makes sense.
> > Relevant question for this PR: Should we permit the libc++ extension type `std::vector<const X>` to actually `push_back` and reallocate when `is_trivially_relocatable_v<X>`? In the past that was obviously impossible, but now it's kinda possible.
> 
> My personal opinion is that this is too dangerous: people will rely on it by accident, not even knowing what they're doing (e.g. templated code with levels of indirection), and that would make it significantly harder to remove or change `__is_trivially_relocatable`.

Good point. It would also be a library API extension that was supported only on Clang 14+ and not other compilers, which would be weird (even if it is an extension //to// an extension). Suggestion withdrawn.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119385