[libcxx-commits] [PATCH] D151953: [libc++] Fix std::copy and std::move for ranges with potentially overlapping tail padding

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__string/constexpr_c_functions.h:149
+      for (; __count > 0; --__count)
+        __dest[__count - 1] = __src[__count - 1];
+    } else {
----------------
hans wrote:
> alexfh wrote:
> > ldionne wrote:
> > > alexfh wrote:
> > > > Some of our (and third-party) code started triggering an error related to this line after this commit. It turns out, std::vector<T>::erase() doesn't compile now, if T is move-only: https://gcc.godbolt.org/z/Txnc66M97
> > > > 
> > > > Is this intended?
> > > Hmm, thanks for the heads up. `vector<T>::erase(...)` should only require that `T` is move assignable. In fact, it looks like this is being triggered in the `std::move` algorithm -- we probably don't have tests with move-only types in `std::move`, which is pretty embarrassing. @philnik it looks like we need to check additional constraints before calling `__constexpr_memmove`. Using `std::move(__src[__count - 1])` here wouldn't be an option since the semantics wouldn't match those of `memmove` IMO.
> > If this is not intended, I'd ask for a revert, since the impact of this is rather large.
> +1 this is breaking us (Chromium) also.
See D154613. This patch is old enough that we'd like to avoid reverting, especially since we have a clear understanding of the issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151953