feat(ff): add div_rem to BigInteger#1110
Open
pjdurden wants to merge 1 commit into
Open
Conversation
pjdurden
requested review from
Pratyush,
WizardOfMenlo and
weikengchen
and removed request for
a team
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a div_rem API to BigInteger and implements it for BigInt<N>, with accompanying tests and release notes.
Changes:
- Introduce
BigInteger::div_rem(&self, &Self) -> (Self, Self)with docs and examples. - Implement base-2 long division in
BigInt<N>to produce quotient and remainder. - Add randomized + edge-case tests using
num_bigint::BigUintas an oracle, and document in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| ff/src/biginteger/mod.rs | Adds the div_rem trait method and implements it for BigInt<N>. |
| ff/src/biginteger/tests.rs | Adds property/edge-case tests for div_rem and wires them into the existing test harness. |
| CHANGELOG.md | Notes the new div_rem API addition for ark-ff. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pjdurden
force-pushed
the
pj/biginteger-div-rem
branch
from
e5881c3 to
cc8800c
Compare
Add a div_rem method to the BigInteger trait returning the quotient and remainder of division by another BigInteger, so callers no longer need to detour through num_bigint::BigInt for division. The implementation generalizes the existing const_modulo base-2 long division to also accumulate the quotient. Tested against num_bigint as an oracle over 1000 random pairs per BigInteger width, plus edge cases. Closes arkworks-rs#645.
pjdurden
force-pushed
the
pj/biginteger-div-rem
branch
from
cc8800c to
d25b856
Compare
Author
6 tasks
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.
Closes #645.
mulforBigIntegerwas added in #772, but division was still missing, so code likeglv.rsstill has to convert intonum_bigint::BigIntto divide. This adds adiv_remto the trait that returns(quotient, remainder)for division by anotherBigInteger.The implementation reuses the same base-2 long division as the existing
const_modulo!helper (which only computes the remainder), extended to also accumulate the quotient bit-by-bit. No new arithmetic primitives needed.Testing: added a randomized test that checks
div_remagainstnum_bigintas an oracle (quotient and remainder) over 1000 random pairs for everyBigIntegerwidth, plus edge cases (0/n, n/n, dividend < divisor, division by one). Panics on division by zero, matchingconst_modulo!.I left the
glv.rscleanup out of this PR to keep it focused on the primitive — happy to follow up with that (and a wide-dividend variant) if useful.