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

Fix rounding for non-representable numbers #968

Merged
merged 20 commits into from
Apr 5, 2023
Merged

Conversation

mborland
Copy link
Member

Partially addresses re-raised #430. Once CI validates this fix works for llround will propagate to the rest of the functions.

@mborland
Copy link
Member Author

@jzmaddock any idea why this would break the use of pre-compiled headers under MSVC? See: https://github.com/boostorg/math/actions/runs/4447933117/jobs/7810069256?pr=968#step:11:523

@jzmaddock
Copy link
Collaborator

any idea why this would break the use of pre-compiled headers under MSVC?

No... I wonder if there are any unrelated changes effecting us - build system?

BTW I can't help thinking there may be a simpler solution, let me have a think.

@mborland
Copy link
Member Author

The failure is now 9223372036854775807.5 cannot be represented as the target type. I assume this would be dependent on rounding mode but the default will be 9223372036854775808 which is greater than LLONG_MAX.

@jzmaddock
Copy link
Collaborator

Ah.... the test case is wrong: it assumes that the boundary case on 2^d where there are d digits in the long long is always an integer, as is float_prior of that value. That will hold as long as there are fewer digits in the floating point type than the integer, but not otherwise (long double in this case).

So... I guess either only test floats that have fewer digits than the integer type - since that is the bug case - or else we need to figure out how to fix the test case. The latter might be tricky, since we're basically duplicating the functionality of llround to carry out the test! So I'd favour the former for now?

@mborland mborland mentioned this pull request Mar 28, 2023
@mborland
Copy link
Member Author

mborland commented Mar 28, 2023

The PCH issues are likely related to recent changes in B2. I have opened an issue with them: bfgroup/b2#229

@mborland
Copy link
Member Author

mborland commented Apr 4, 2023

@jzmaddock Can you take a look at this now that it's green? The context switching for constexpr calculation is more verbose than I had hoped.

@jzmaddock
Copy link
Collaborator

Thanks for this Matt: other than the two minor comments looks good to me!

@mborland mborland merged commit 9a3b8bc into boostorg:develop Apr 5, 2023
@mborland mborland deleted the 430 branch April 5, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants