On 19/07/18 10:01 -0400, Glen Fernandes wrote:
On Thu, Jul 19, 2018 at 9:25 AM Jonathan Wakely <jwak...@redhat.com> wrote:
Sorry for the delay in reviewing this properly, as I've only just
realised that this introduces undefined behaviour, doesn't it?
It's undefined to use memmove for a type that is not trivially
copyable. All trivial types are trivially copyable, so __is_trivial
was too conservative, but safe (IIRC we used it because there was no
__is_trivially_copyable trait at the time, so __is_trivial was the
best we had).
There are types which are trivially assignable but not trivially
copyable, and it's undefined to use memmove for such types.
I was still unclear about that, but I forwarded you an e-mail from
Marshall with his answer when I asked whether libc++'s use of
TriviallyCopyAssignable here was incorrect. Let me know if it applies
here, and if not (and that interpretation of the standard is
incorrect), I'll update the patch to do as you suggest and run the
tests again.
While I sympathise with Marshall's position (that std::copy only cares
about assignment not copying) that doesn't make it OK to use memmove
here.
Using memmove for a non-trivially copyable type is undefined. Period.
The fact GCC warns that it's undefined also means GCC might start
optimising based on the assumption that undefined behaviour isn't
reached at runtime. So it could (for example) assume that the input
range must be empty and remove the entire call to std::copy.
For a non-trivially copyable, trivially assignable type I think we
just have to rely on the compiler to transform the assignments into
optimal code (which might end up being a memmove, ironically).
Please do update the patch to use __is_trivially_copyable. I don't
think we need the __is_simple_copy_move helper in that case, just
change two uses of __is_trivial to __is_trivially_copyable.
