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

Silenced Visual Studio compiler warning #298

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Conversation

jrade
Copy link
Contributor

@jrade jrade commented Jan 16, 2025

This code caused a C4127 "conditional expression is constant" compiler warning when compiled with Visual Studio at warning level 4 with T a signed integer type.

This code caused a C4127 "conditional expression is constant" compiler warning
when compiled with Visual Studio at warning level 4 with T a signed integer type.
@lemire
Copy link
Member

lemire commented Jan 16, 2025

I have mixed feelings about this PR.

Honestly, the Visual Studio studio warning is just dumb in this instance.

Why are you enabling 4127?

@dalle
Copy link
Collaborator

dalle commented Jan 16, 2025

C4127 is enabled on the highest warning level (4). I'd like to see the library to compile without warnings on the highest level on all compilers out of the box.

https://learn.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compiler-warning-level-4-c4127?view=msvc-170

@dalle
Copy link
Collaborator

dalle commented Jan 16, 2025

Part of it could be solved using constexpr if, but that is available first in C++17.

@lemire
Copy link
Member

lemire commented Jan 16, 2025

Because this is supported by @dalle, we will pull it in.

Running tests.

@jrade
Copy link
Contributor Author

jrade commented Jan 16, 2025

I agree 100% with Dalle's comments.

@lemire
Copy link
Member

lemire commented Jan 16, 2025

Will be part of the release!

@lemire lemire merged commit 482cc1f into fastfloat:main Jan 16, 2025
37 checks passed
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.

3 participants