-
Notifications
You must be signed in to change notification settings - Fork 359
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
[Compilation] Compile warnings using GCC 14 signedness and missing braces #131
Comments
Hello, and thanks for opening this issue!
Absolutely, it's early days yet and getting objectively incorrect things fixed is always going to be a priority over maintaining strict backward compatibility. I will fix any stragglers after #118 is merged. |
We're ignoring missing braces for now, and I will address the sign compare in the coming days 👍 |
PR for fixing the signed comparisons is up here: #152 |
PR has been merged, please let me know if it fixes the issue for you 👍 |
This does fix the warnings. Thanks. I was fixing them myself initially, thinking I could do it before some family would visit. I don't think I was half way when they showed up :laugh:. |
The current examples cannot be compiled on a Linux/Arch machine using GCC 14 with the compiler arguments in the CMakeLists.txt files.
The main culprit being
-Werror=sign-compare
and-Werror=missing-braces
implicitly added by-Wall
or-Wextra
and made an error with-Werror
.Proposals:
silence the errors by either adding some nasty
#pragma
s or slightly better: add-Wno-error=sign-compare
and-Wno-error=missing-braces
to the C flags (not replacing the CXX flags)But preferably, these violations ought to be fixed. IMO a lot of those signed values don't make sense, because it's not checking for or using the properties of those negative values, which would break a lot I presume.
Is it in the cards to make an API change at this level? I like how you're using int32_t instead of int for portability, but I think the function signatures should often not contain signed integer arguments, if ever, so use uint32_t (or often size_t) instead.
Brief example: https://godbolt.org/z/GbqzxjboW
The text was updated successfully, but these errors were encountered: