-
Notifications
You must be signed in to change notification settings - Fork 17.2k
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
Stop using -fsingle-precision-constant #5620
Comments
nice idea ! I see those warning (mostly in gps and location). Another solution could be to make them static const double instead of define ! |
@khancyr yes, I agree. But it's just a matter of time until more appear. I don't like these surprises because we have a flag to deviate from the standard. Also, this makes the behavior different on sitl/linux than what it is on stm32 boards. |
I'll ask @CraigElder to add to the agenda for the next dev call? |
Thanks Randy. Added |
As discussed today on the dev call feedback was that this is not desired because would need to change the codebase to add the f suffix and new people get it wrong which would cause operations on double variables rather than float. |
This is the usual case in which the remedy creates more problems than it's solving. By passing
-fsingle-precision-constant
to the compiler we make literals in the code to be interpreted as float rather than double. The problem is that there's no way to tell that a literal is in fact double so in cases in which it's needed, it's a) truncated to float and b) cause a warning of implicit float to double promotion when involved in other operations. This is what happens today onlibraries/AP_Math/location.cpp
.It's also the case of least surprise in which C++ developers already expect that to be true. It's very easy to learn for those who don't know, makes our code "less special" and the warning
-Wdouble-promotion
is there to help mitigate the problems.Plan to solve it:
With this in place we should be able to eliminate the remaining implicit float to double conversions.
The text was updated successfully, but these errors were encountered: