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

Naming conventions for widening/split operations #537

Open
tarcieri opened this issue Dec 29, 2023 · 1 comment
Open

Naming conventions for widening/split operations #537

tarcieri opened this issue Dec 29, 2023 · 1 comment

Comments

@tarcieri
Copy link
Member

There are two different conventions for widening operations used in this crate:

Notably square_wide returns a "split" result, rather than widening the output, which is called Uint::split_mul for multiplication.

Should square_wide be renamed to split_square for consistency with split_mul? Or should they be renamed to something else?

@tarcieri
Copy link
Member Author

tarcieri commented Feb 7, 2024

The proposed bigint helper methods in core (rust-lang/rust#85532) use the following names:

  • carrying_add
  • borrowing_sub
  • carrying_mul
  • widening_mul

Perhaps it would be good to generally align names and semantics with those. Names like adc and sbb that are derived from CPU instructions are on the one hand nicely terse and on the other perhaps a bit unclear unless you're already familiar with the CPU instructions.

They also use these type signatures:

/// `self + rhs + carry` (full adder)
fn carrying_add(self, rhs: Self, carry: bool) -> (Self, bool);

/// `self - rhs - carry` (full "subtractor")
fn borrowing_sub(self, rhs: Self, carry: bool) -> (Self, bool);

/// `self * rhs + carry` (multiply-accumulate)
fn carrying_mul(self, rhs: Self, carry: Self) -> (Self, Self);

/// `self * rhs` (wide multiplication, same as `self.carrying_mul(rhs, 0)`)
fn widening_mul(self, rhs: Self) -> (Self, Self);

Notably carrying_mul and widening_mul return a double-width result as two instances of Self, which is what the latest prereleases of crypto-bigint define as split_mul.

So perhaps split_mul should be renamed to widening_mul, and the current widening_mul should be renamed to something else.

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

No branches or pull requests

1 participant