some -Wdouble-promotion Clang compiler warnings
#4820
Conversation
lib/programmemory.cpp
Outdated
| ValueFlow::Value v; | ||
| combineValueProperties(args[0], args[1], &v); | ||
| v.floatValue = std::nexttoward(value, args[1].isFloatValue() ? args[1].floatValue : args[1].intvalue); | ||
| v.floatValue = std::nexttoward(value, static_cast<long double>(args[1].isFloatValue() ? args[1].floatValue : args[1].intvalue)); |
There was a problem hiding this comment.
I don't understand what we accomplish with this explicit cast. Technically the code is the same right?
I can understand removing trailing f from some float literal if it's assigned to a double.
There was a problem hiding this comment.
In my experience .. there is dangers with casts. There can be loss of precision/sign and stuff and the compiler will be silent about the problems. I am skeptic about adding casts to silence the compiler.
There was a problem hiding this comment.
The compiler is silence anyways since we decided to turn off these warnings.
The compiler will only warn about implicit casts. So adding explicit casts is not about silencing the compiler but showing that the cast is (or has to be in case of mismatching types/signatures) intentional and not a mistake.
The goal would be to have no casts at all and we achieved that in all cases but one by providing the proper type. And that case cannot be solved since our types do not match that one the function expects. Hence the explicit cast to highlight that.
There was a problem hiding this comment.
The compiler will only warn about implicit casts. So adding explicit casts is not about silencing the compiler but showing that the cast is (or has to be in case of mismatching types/signatures) intentional and not a mistake.
You add the cast and today it does nothing technically. So you think it's safe.
Then some years later some some types are changed in the code and your cast will cause loss of sign/precision, or something important and the compiler is silent about it because you have an explicit cast.
Can you tell me that your casts will have no effect technically in 20 years when various types have changed.
There was a problem hiding this comment.
You add the cast and today it does nothing technically. So you think it's safe.
That's why I stated there should be no casts at all. You should always use the actual type that will be used at the end. Unfortunately even the standard has several shortcomings where that isn't possible with clean and simple code.
Then some years later some some types are changed in the code and your cast will cause loss of sign/precision, or something important and the compiler is silent about it because you have an explicit cast.
That should not be the case. We would be only be adding explicit casts in terms where the precision, range or signedness would change. And if that parameter changes and the cast would again cause such an issue it would be detected.
The more likely issue is if the type of the variable you are calling the function with changes since you always force that into a type. That would be suppressed forever.
But you have issues either way.
If you don't add the cast you don't know that the value might change silently (as it actually happened with the 2.2 vs 2.2f constants).
So we could just fix the warnings that do not require any casts and leave it off but then new code might introduce new issues with detecting it and we would have to do regular runs with the warnings. It is unfortunately that suppressing compiler warnings is so much more complex and we cannot simply add an attribute to those cases.
On a side note - enabling the -fsanitize=integer run-time checks might also help with this. See #2922.
There was a problem hiding this comment.
I reverted this change and disabled the warning again.
…adjusted some `TestValueFlow` test cases
firewave
changed the title
-Wdouble-promotion Clang compiler warnings-Wdouble-promotion Clang compiler warnings
2 participants
No description provided.