-
-
Notifications
You must be signed in to change notification settings - Fork 953
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
weather: Fix incorrect rounding for negative temperatures #2194
base: main
Are you sure you want to change the base?
Conversation
Build size and comparison to main:
|
Would it be worth extracting out rounding integer division to some shared code? It's somewhat annoying that this isn't part of the C++ stdlib really :( Maybe an implementation similar to this? https://github.com/lucianpls/Rounding-Integer-Division |
Make weather service use it.
What do you think of this? I've made a RoundedDiv function in the Utility namespace, that uses C++20 concepts so that we can have keep the code type-agnostic, while making sure that it only works for expected values. Didn't you also use a rounded division somewhere in the AoD code? Would this solution also work there? |
Wow that looks great. I didn't know that concepts allow this, I should read up on it. Yes I think this would work for AOD, I guess that would go in a separate PR though? I want to double check correctness properly before approving, but otherwise LGTM |
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.
I think this doesn't work as C++ demotes the dividend from signed to unsigned when the divisor is unsigned:
Suppose the following
int a;
// -1 / 5, rounded
a = (-1 + (-1 >= 0 ? 5 : -5) / 2) / 5;
// a = 0 as expected
a = (-1 + (-1 >= 0 ? 5U : -5U) / 2) / 5U;
// uh oh, a = 429496728
// more simply
a = -1 / 2u;
// a = 2147483647
I didn't even know this was a thing??
Oh yeah of course. Integer promotion rules are really stupid. This conversion to unsigned also only happens when the values are |
of course... 😅 I love the C++ spec too |
Thanks @tokiwee for reporting this issue!
Closes #2185 (if GitHub un-deletes it).