Skip to content

Conversation

@kennykerr
Copy link
Collaborator

@kennykerr kennykerr commented Feb 12, 2020

C++/WinRT is used in places where memcpy is no longer permitted.

@kennykerr kennykerr changed the title Fix OS build break Avoid using memcpy Feb 12, 2020
@kennykerr kennykerr merged commit 3307d54 into master Feb 12, 2020
@kennykerr kennykerr deleted the kennykerr-memcpy branch February 12, 2020 14:38
@walbourn
Copy link
Member

walbourn commented Feb 12, 2020

FWIW I've had some complaints from people on my GitHub projects about using memcpy_s instead of memcpy on some toolsets. Also, not all compilers treat memcpy_s as an intrinsic where it does for memcpy.

But officially memcpy_s is part of C11 and therefore it's part of C++17, so for C++/WinRT it's probably a non-issue.

@kennykerr
Copy link
Collaborator Author

Yeah, its a silly restriction in my opinion. There's nothing more "secure" about memcpy_s. Ideally we could just use std::copy but VC's implementation gets confused by array vs pointer types.

@DefaultRyan
Copy link
Member

DefaultRyan commented Feb 12, 2020

Last I checked VC doesn't get confused about array vs pointer types - it checks the size of the output array and fires a runtime assert if it's too small for the incoming data. This is easily dodged by casting the buffer from an array to a pointer before calling std::copy_n, like this:
std::copy_n(value, length, static_cast<wchar_t*>(header->buffer));

Here's two comparisons with optimizations disabled, and _DEBUG defined to light up STL iterator debugging.
Before:
https://godbolt.org/z/_2zj5R

After:
https://godbolt.org/z/-Udg6P

Still calling memmove. but note that the "after" disassembly no longer has that call to _CrtDbgReport

There's no reason for us to avoid std::copy_n.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants