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

Add SubResidue trait and impls for Residue and DynResidue #149

Merged
merged 1 commit into from
Dec 21, 2022

Conversation

fjarri
Copy link
Contributor

@fjarri fjarri commented Dec 15, 2022

Not sure about adding neg():

  • Should there be a separate NegResidue trait, or should it be a part of SubResidue?
  • The default implementation (via SubResidue) would require an access to zero, and therefore an additional trait bound, which DynResidue does not implement.

@fjarri fjarri force-pushed the monty-sub branch 2 times, most recently from d9ad0e0 to 7919364 Compare December 15, 2022 04:17
@tarcieri
Copy link
Member

Should there be a separate NegResidue trait, or should it be a part of SubResidue?

A separate NegResidue trait seems fine to me

@fjarri
Copy link
Contributor Author

fjarri commented Dec 20, 2022

Do you want me to add NegResidue in this PR, or leave it for another one?

@tarcieri
Copy link
Member

Seems fine in this PR

@fjarri
Copy link
Contributor Author

fjarri commented Dec 20, 2022

Speaking of, why are there separate *Residue traits at all? Wouldn't the core::ops ones suffice?

@tarcieri
Copy link
Member

Yeah, I've had similar thoughts but never broached them. We should probably switch to the core::ops traits.

cc @jellevos

@tarcieri tarcieri merged commit 9316a1d into RustCrypto:master Dec 21, 2022
@tarcieri
Copy link
Member

@fjarri perhaps you can follow up on switching to the core::ops traits and impl'ing Neg in that PR?

@fjarri fjarri deleted the monty-sub branch December 21, 2022 20:06
@jellevos
Copy link
Contributor

Yeah, I've had similar thoughts but never broached them. We should probably switch to the core::ops traits.

cc @jellevos

Yes, in hindsight, that makes total sense. I saw the great progress you two made in the past weeks! Happy to help out if anything still needs to be taken care of regarding modular arithmetic.

@tarcieri tarcieri mentioned this pull request Feb 27, 2023
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.

3 participants