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

enabled and fixed -Wdouble-promotion Clang warnings #7164

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

firewave
Copy link
Collaborator

@firewave firewave commented Jan 2, 2025

No description provided.

@danmar
Copy link
Owner

danmar commented Jan 3, 2025

Clang/GCC doesn't implement their checkers properly...

Code 1:

void dostuff(double x);

void f() {
    dostuff(0.1f);
}

Clang says:

test1.c:5:13: warning: implicit conversion increases floating-point precision: 'float' to 'double' [-Wdouble-promotion]

Code 2:

void dostuff(double x);

void f() {
    dostuff(static_cast<double>(0.1f));
}

.. after some time you have this ..

Code 3:

void dostuff(float x);

void f() {
    dostuff(static_cast<double>(0.1f));
}

And the compilers are silent about this. By turning on -Wdouble-promotion this is what you get. There was no problem in Code 1 but in Code 3 there is a performance problem. And it hurts the readability.

@firewave
Copy link
Collaborator Author

firewave commented Jan 3, 2025

Clang/GCC doesn't implement their checkers properly...

In that case please file an upstream report.

Looks like this might also be related to llvm/llvm-project#93288.

@danmar
Copy link
Owner

danmar commented Jan 5, 2025

In that case please file an upstream report.

Nah, I doubt they would care about it.

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reject this because it can lead to performance and readability issues in the long run.

@@ -824,7 +824,7 @@ static std::unordered_map<std::string, BuiltinLibraryFunction> createBuiltinLibr
return ValueFlow::Value::unknown();
ValueFlow::Value v;
combineValueProperties(args[0], args[1], v);
v.floatValue = std::nexttoward(asFloat(args[0]), asFloat(args[1]));
v.floatValue = std::nexttoward(asFloat(args[0]), static_cast<long double>(asFloat(args[1])));
Copy link
Owner

@danmar danmar Jan 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this "fix" the issue.. there is still the promotion that clang warned about. This code will work technically exactly the same as before as far as I see.

@firewave
Copy link
Collaborator Author

firewave commented Jan 5, 2025

Nah, I doubt they would care about it.

Great attitude. Imagine people had the attitude towards false positives in Cppcheck.

Well, you might not imagine. They have that 100%. They just suppress the FP and if at some point there are too many suppressions or it gets too slow they just completely drop it completely because it "offers no value". That's how any tooling (not just open source) starts to deteriorate (even in cases where the devs actually care about the application and bug reports).

Well, they basically were too lazy to file a report and found me as the idiot which did their work for them because I cared.

That is actually the scenario because I got involved again because I wanted it to improve, so other people see that it actually has value and should be kept. See where that got me ...

And there has also been cases where people (including me) made something out to be a "false positive" but it turned out it was not and at the end you actually learned something new....

@danmar
Copy link
Owner

danmar commented Jan 10, 2025

Well, you might not imagine. They have that 100%.

Yes I have seen it myself also.

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.

2 participants