-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix: floating point literals must use the f suffix when working with float variables #1873
Conversation
LGTM, but I'll wait abit before committing. |
src/devices/acurite.c
Outdated
@@ -729,7 +729,7 @@ static int acurite_txr_decode(r_device *decoder, bitbuffer_t *bitbuffer) | |||
// 11 bits needed for specified range -40 C to 70 C (-40 F - 158 F) | |||
// range -100 C to 1538.4 C | |||
int temp_raw = ((bb[4] & 0x7F) << 7) | (bb[5] & 0x7F); | |||
tempc = temp_raw * 0.1 - 100; | |||
tempc = temp_raw * 0.1f - 100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is still a double lurking here. prefered would be (temp_raw - 1000) * 0.1f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or maybe it's an implicit float…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to VC++, it gets indeed converted to a float
not a double
, there is no longer a warning on that line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I hesitated changing 100
to 100.0f
but I'm not sure it makes it any clearer...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
temp_raw should be converted to float and then -100 should be converted to float and then everything is float
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my point was an integer substraction, then a float multiply instead of two float ops.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rules give that it should be float. But I see your point if the int is large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial goal of that PR was to address the warnings by simply adding the suffix wherever required.
That being said, I agree that there are optimization opportunities at various places (value / 3600.0f / 1000.0f
comes to mind) but shouldn't this be addressed in another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, and greping for f -
afterwards will find most of those instances.
8e31ece
to
e42ad33
Compare
I have just rebased this branch on the latest master, please let me know if this is still of interest |
4e3824b
to
6cc1af6
Compare
93031a1
to
ed794ec
Compare
…on by using a float literal
This looks ready to merge to me, by code review. |
This fixes C4244/C4267/C4305 warnings related to
double
tofloat
conversions when a literal is involved in the warning, as reported in #1866