search

microsoft / DirectXTK

The DirectX Tool Kit (aka DirectXTK) is a collection of helper classes for writing DirectX 11.x code in C++
https://walbourn.github.io/directxtk/
MIT License
2.55k stars 506 forks source link

SDL - banned API memcpy #146

Closed tosmolka closed 5 years ago

tosmolka commented 5 years ago

Hi, we noticed that DirectX Tool Kit in few places uses functions (e.g. memcpy) that we try to globally ban in our projects. In case of memcpy it seems that parts of the kit already uses memcpy_s, can you please prioritize transforming the rest? Thank you.

walbourn commented 5 years ago

I don't use _CRT_SECURE_NO_WARNINGS in my projects, nor do I globally disable the C4996 warning, so in general I'm not using 'SDL banned APIs'.

I'm not using _CRT_SECURE_WARNINGS_MEMORY because not all system headers are actually compatible with that and they generate 4996 warnings.

What functions other than memcpy are you concerned about? Are you using DirectXTK for Audio or no?

tosmolka commented 5 years ago

Hi Chuck, our SDL tooling is flagging only memcpy in WICTextureLoader.cpp. I think usage there is fine but since it's just couple of places, would be nice to avoid it if possible. Thanks.

walbourn commented 5 years ago

Seems legit to use memcpy_s for file parsing code, so sure.

walbourn commented 5 years ago

Yeah, it seems odd to do memcpy_s when the parameter for both DestinationSize and SourceSize is the same thing. It does make sense if they are two different things...

Still for WICTextureLoader it's easy enough to make it:

memcpy(&convertGUID, &GUID_WICPixelFormat32bppRGBA, sizeof(WICPixelFormatGUID));

->

memcpy_s(&convertGUID, sizeof(WICPixelFormatGUID), &GUID_WICPixelFormat32bppRGBA, sizeof(GUID));
walbourn commented 5 years ago

See this commit.

Also looking at other GitHub projects.

tosmolka commented 5 years ago

Thanks Chuck, I agree memcpy was fine in this case - thanks anyway for the change.