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

Define extended GCD, combined GCD+LCM #19

Merged
merged 9 commits into from
Apr 25, 2019
Merged

Define extended GCD, combined GCD+LCM #19

merged 9 commits into from
Apr 25, 2019

Conversation

strake
Copy link
Contributor

@strake strake commented Dec 18, 2018

No description provided.

@strake
Copy link
Contributor Author

strake commented Mar 21, 2019

Rebased, PTAL

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please cargo fmt this, and if it skips the macro impls, please format them manually.

cc #10 and #12 that overlap this PR.

src/lib.rs Outdated
#[cfg_attr(array_clone, derive(Clone))]
pub struct ExtendedGcd<A> {
pub gcd: A,
pub coeffs: [A; 2],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use separate x and y, so we can document it something like this:

Bézout's identity — Let a and b be integers with greatest common divisor d. Then, there exist integers x and y such that ax + by = d.

Then we also don't have to worry about array_clone.

src/lib.rs Outdated
let q = r.1.clone() / r.0.clone();
let f = |r: &mut (Self, Self)| {
mem::swap(&mut r.0, &mut r.1);
r.0 -= q.clone() * r.1.clone();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it's a little annoying to have SubAssign in the API just for this, and AFAICT it's only needed because you're working through &mut. If the closure used values instead, or if you just flattened these three calls, then you could move and then reassign r.0 just fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should require Signed though -- I believe it's useless for unsigned integers, since one of the coefficients must be negative, except the degenerate case with zeros.

@strake
Copy link
Contributor Author

strake commented Apr 10, 2019

@cuviper All done, PTAL

@cuviper
Copy link
Member

cuviper commented Apr 25, 2019

Thanks!

bors r+

bors bot added a commit that referenced this pull request Apr 25, 2019
19: Define extended GCD, combined GCD+LCM r=cuviper a=strake



Co-authored-by: M Farkas-Dyck <strake888@gmail.com>
Co-authored-by: Josh Stone <cuviper@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 25, 2019

Build failed

@cuviper
Copy link
Member

cuviper commented Apr 25, 2019

Sorry, that was my fault.

bors r+

bors bot added a commit that referenced this pull request Apr 25, 2019
19: Define extended GCD, combined GCD+LCM r=cuviper a=strake



Co-authored-by: M Farkas-Dyck <strake888@gmail.com>
Co-authored-by: Josh Stone <cuviper@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 25, 2019

Build succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants