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.

Sign up for GitHub

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

Jump to bottom

BigInt delegates for inputs not in domain #9797

Merged
merged 1 commit into from Jan 31, 2022
Merged

BigInt delegates for inputs not in domain #9797

merged 1 commit into from Jan 31, 2022

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Oct 29, 2021 •
edited by lrytz

It errors identically as the underlying BigInteger.

Follow-up for #9628

@som-snytt
BigInt delegates for inputs not in domain
2d36e71 
It errors identically as the underlying BigInteger.
@scala-jenkins scala-jenkins added this to the 2.13.8 milestone Oct 29, 2021
lrytz
lrytz previously approved these changes Nov 18, 2021
View reviewed changes
@lrytz
Copy link
Member

lrytz commented Nov 18, 2021

Thanks!

@lrytz lrytz dismissed their stale review November 18, 2021 12:16

...thinking

@lrytz
Copy link
Member

lrytz commented Nov 18, 2021 •
edited

Though I wonder what's the risk of existing code relying on the existing implementation being too permissive in long-ranged BigInts? BigInt(7) mod BigInt(-4) was OK before. But it's probably better to make that code fail fast, as it would fail when the numbers grow out of the long range.

Also, I just saw the BigInt.% method still (after this PR) allows a negative modulus.

@dwijnand dwijnand modified the milestones: 2.13.8, 2.13.9 Dec 15, 2021
@SethTisue SethTisue modified the milestones: 2.13.8, 2.13.9 Dec 15, 2021
@SethTisue
Copy link
Member

@Ichoran is this one you'd be interested in having a look at? and/or @NthPortal, @denisrosset ?

@SethTisue SethTisue added the library:base Changes to the base library, i.e. Function Tuples Try label Jan 19, 2022
@NthPortal
Copy link
Contributor

honestly, the permissiveness is only due to the recent changes if it fits within a Long, so as far as I'm concerned, we're fixing a recently introduced bug

@NthPortal
Copy link
Contributor

Also, I just saw the BigInt.% method still (after this PR) allows a negative modulus.

@lrytz % is remainder, not modulus. I know that seems pedantic, and I know that everyone refers to % as modulus, but it isn't. technically, modulus shouldn't even allow a negative rhs, but I'm not going to attempt to litigate that against Java and the rest of the programming world

@lrytz
Copy link
Member

lrytz commented Jan 31, 2022

recent changes if it fits within a Long

👍 thanks for the reminder. #9628

% is remainder, not modulus

🤭 you can be glad that i'm not working with numbers...

@lrytz lrytz merged commit 7fe12bc into scala:2.13.x Jan 31, 2022
@som-snytt som-snytt deleted the issue/bigint-parity branch August 1, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NthPortal NthPortal NthPortal approved these changes

@lrytz lrytz lrytz left review comments

Assignees
No one assigned
Labels
library:base Changes to the base library, i.e. Function Tuples Try
Projects
None yet
Milestone
2.13.9
6 participants
@som-snytt @lrytz @SethTisue @NthPortal @dwijnand @scala-jenkins