Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
sbd1138 opened this issue Mar 3, 2023 · 12 comments
Closed
Assignees
Labels
complier Issue related to compiler codegen

Comments

@sbd1138
Copy link
Contributor

sbd1138 commented Mar 3, 2023

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
Copy link
Member

walbourn commented Mar 3, 2023

Which version of GCC are you using?

@sbd1138
Copy link
Contributor Author

sbd1138 commented Mar 3, 2023

9.4.0 (compiling on/via WSL2, FWIW).

@sbd1138
Copy link
Contributor Author

sbd1138 commented Mar 5, 2023

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
Copy link
Member

walbourn commented Mar 5, 2023

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.

@walbourn walbourn pinned this issue Mar 5, 2023
@walbourn walbourn self-assigned this Mar 5, 2023
@walbourn walbourn added the conformance Related to supporting non-MSVC compilers label Mar 5, 2023
@sbd1138
Copy link
Contributor Author

sbd1138 commented Mar 5, 2023

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
Copy link
Member

walbourn commented May 7, 2023

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

@sbd1138
Copy link
Contributor Author

sbd1138 commented May 7, 2023

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
Copy link
Member

walbourn commented May 8, 2023

Ah, thanks. I'll take a look.

@walbourn
Copy link
Member

walbourn commented May 8, 2023

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

@walbourn walbourn closed this as completed May 8, 2023
@walbourn walbourn unpinned this issue May 8, 2023
@sbd1138
Copy link
Contributor Author

sbd1138 commented May 8, 2023

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

@walbourn walbourn added complier Issue related to compiler codegen and removed conformance Related to supporting non-MSVC compilers labels May 8, 2023
@sbd1138
Copy link
Contributor Author

sbd1138 commented May 11, 2023

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
Copy link
Member

OK. Thanks for the follow-up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complier Issue related to compiler codegen
Projects
None yet
Development

No branches or pull requests

2 participants