Merged
Conversation
Member
|
Too lazy to try and follow that reasoning, but does that also work if |
Member
Author
|
Good catch! No, the story is much worse then because the |
LilithHafner
commented
Nov 9, 2022
Member
|
Brava, @LilithHafner 👏🏻 |
Member
Author
|
Unless folks object, I'm going to merge this. The reasoning justifying that the new behavior is always correct is complex, but it is pretty clear that this is at least an improvement. |
Member
|
go for it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixup for #46764. See the post-merge conversation for context. Reposting the comment that motivates this pr:
There are two ways that pow5 can be wrong,
m2is divisible bybig(5)^requiredFivesm2is not divisible bybig(5)^requiredFivesIn the first case, for pow5 to be wrong,
big(5)^requiredFivesmust be greater thantypemax(Int), butm2is always less than1<<53, and a small number is not divisible by a large number. Thusm2will never be divisible by the truebig(5)^requiredFiveswhen there is overflow and case 1 is okay.For the second case, we need to find a
requiredFivessuch thatpow5may wrongly return true. That is, find arequiredFivessuch that there existsm2wherem2 % 5^requiredFives == 0For this to be the case, eitherm2must be 0 (handled much earlier as a special case) orabs(5^requiredFives) <= m2 < 1<<53. Searching exhaustively from the cases that have overflow,findfirst(requiredFives -> abs(5^requiredFives) < 1<<53, 28:10^5)+27 == 55. Note that5^55 < 0. If, on the other hand, we usedunsinged(5)^requiredFives, then we'd havefindfirst(requiredFives -> unsigned(5)^requiredFives < 1<<53, 28:10^5)+27 == 1048and IIUCrequiredFivesis at mostlog10(floatmax(Float64)) == 308.25471555991675.So using
unsigned(5)should fix this and I benchmark it as comparable to signed exponentiation.