microsoft / DirectXMath

DirectXMath is an all inline SIMD C++ linear algebra library for use in games and graphics apps
https://walbourn.github.io/introducing-directxmath/
MIT License
1.54k stars 236 forks source link

DirectXPackedVector.h/.inl Linux compilation warnings (Clang and GCC) #161

Closed sbd1138 closed 1 year ago

sbd1138 commented 1 year ago

DirectXPackedVector.h/.inl currently exhibit compile warnings/errors when attempting to include them on Linux builds using both GCC and Clang.

Both GCC and Clang bark about #pragma warnings that can be fixed with some #ifdef guards. GCC also has errors relating to type conversions, all of them being of this nature:

DirectXPackedVector.h(373,79): error : conversion from ‘uint16_t’ {aka ‘short unsigned int’} to ‘unsigned char:5’ may change value [-Werror=conversion]

As a side note, I might also propose moving XMConvertHalfToFloat and XMConvertFloatToHalf to DirectXMathConvert.h/inl (these are the functions I am attempting to use). Semantically it does make sense as they are more general conversion functions that have use outside the scope of the packed vector code (I'm using them when reading back some F16 texture data on the CPU).

walbourn commented 1 year ago

Which version of GCC are you using?

sbd1138 commented 1 year ago

9.4.0 (compiling on/via WSL2, FWIW).

sbd1138 commented 1 year ago

DirectXPackedVector.h(373,79): error : conversion from ‘uint16_t’ {aka ‘short unsigned int’} to ‘unsigned char:5’ may change value [-Werror=conversion]

constexpr XMU565(uint8_t _x, uint8_t _y, uint8_t _z) noexcept : x(_x), y(_y), z(_z) {}

This conversion error is a little bit interesting. It seems to imply that the compiler has implicitly converted the uint16_t x : 5 bitfield into an unsigned char, and then it's complaining about that conversion? The arguments to the constructor are already uint8_t, so it wouldn't appear those are the issue.

slightly confused dog sideways head.jpg

Or perhaps it's doing an implicit conversion of the constructor argument to uint16_t (the base type of the bitfield)?

walbourn commented 1 year ago

There's something odd about the bit-field here that's probably related to a nuance of the C++ Standard. I'll take a look.

sbd1138 commented 1 year ago

From searching around online, it appears this bitfield initialization conversion error in GCC has been problematic for many. I did try masking the arguments (as it seems in other cases the compiler takes note and doesn't complain), but no dice. Further, even just ignoring the arguments and trying to just have : v(0) as the only initializer still generates the error. So there's definitely something interesting happening here.

walbourn commented 1 year ago

Which version of DirectXMath are you using? All the unknown pragma warnings were cleaned up in #133.

sbd1138 commented 1 year ago

I do have those fixes for #133, but note that the above issues are specifically in DirectXPackedVector.h/.inl, which are not included via DirectXMath.h. It was only once I started to use/include DirectXPackedVector.h that I discovered these as-yet-undiscovered warnings.

walbourn commented 1 year ago

Ah, thanks. I'll take a look.

walbourn commented 1 year ago

I don't get any warnings with GCC 11.3. It looks like your -Wconversion error is fixed in newer builds of the compiler.

sbd1138 commented 1 year ago

Thanks for taking a look...I'll see about updating my GCC install to the latest.

sbd1138 commented 1 year ago

FYI this error was still present in GCC 11.1 (the latest packaged version I could update to). So I'll just keep the warning disabled and periodically check newer available versions. Annoyingly, newer GCC has some issues with erroneous array bounds errors, so I'll likely be staying on 9.4.0 for the time being.

walbourn commented 1 year ago

OK. Thanks for the follow-up!