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

Use in_place suffix for BigInteger operations #781

Open
tcoratger opened this issue Feb 15, 2024 · 1 comment · May be fixed by #782
Open

Use in_place suffix for BigInteger operations #781

tcoratger opened this issue Feb 15, 2024 · 1 comment · May be fixed by #782

Comments

@tcoratger
Copy link
Contributor

In the whole codebase, there is a standard which is to include the suffix in_place on operations which modify the value passed as a mutable parameter. For example here:

fn double_in_place(&mut self, two_inv: &P::Fp) -> EllCoeff<P> {
// Formula for line function when working with
// homogeneous projective coordinates.
let mut a = self.x * &self.y;
a.mul_assign_by_fp(two_inv);
let b = self.y.square();
let c = self.z.square();
let e = P::G2Config::COEFF_B * &(c.double() + &c);
let f = e.double() + &e;
let mut g = b + &f;
g.mul_assign_by_fp(two_inv);
let h = (self.y + &self.z).square() - &(b + &c);
let i = e - &b;
let j = self.x.square();
let e_square = e.square();
self.x = a * &(b - &f);
self.y = g.square() - &(e_square.double() + &e_square);
self.z = b * &h;
match P::TWIST_TYPE {
TwistType::M => (i, j.double() + &j, -h),
TwistType::D => (-h, j.double() + &j, i),
}
}

We know from the function name that the self mutable will be modified.

However similar operations are performed in the BigInteger trait like:

fn mul2(&mut self) -> bool;

or

fn sub_with_borrow(&mut self, other: &Self) -> bool;

without including the in_place suffix. For readability and consistency, it could be nice to include this suffix for all the BigInteger operations concerned by this type of logic.

@Pratyush
Copy link
Member

Yes, this would be great!

@tcoratger tcoratger linked a pull request Feb 16, 2024 that will close this issue
6 tasks
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 a pull request may close this issue.

2 participants