search

RustCrypto / crypto-bigint

Cryptography-oriented big integer library with constant-time, stack-allocated (no_std-friendly) implementations of modern formulas
Apache License 2.0
182 stars 51 forks source link

Make the `Monty::new` borrow the `Uint` #600

Closed dvdplm closed 4 months ago

dvdplm commented 4 months ago

Change the Monty trait's new method to take a ref to the Integer instead of ownership.

Fixes #599

tarcieri commented 4 months ago

@fjarri I'm guessing this will need changes in crypto-primes

fjarri commented 4 months ago

My personal approach is to take by reference the arguments that are going to be processed in some way, and by value the ones that are going to be used verbatim (e.g. saved in a struct). So for this method I would vote for passing the argument by reference.

But in the boxed case, while the value of the integer is not used verbatim, the heap memory where it's stored is. I can see the value of that to avoid an allocation. Would it be useful to have two new() methods - one that takes a reference and one that takes a value? new(&) and new_consuming() or something?

dvdplm commented 4 months ago

I quite like having two separate initializers, reflecting more explicitly what is going on under the hood.

tarcieri commented 4 months ago

Two initializers where one just calls clone() for you? I think it would be simpler to have one and let the caller clone if need be, which I guess is the current state of affairs.

(When I made my original comment about changing this to a reference, I guess I forgot that BoxedMontyForm::new doesn't borrow and tries to reuse the memory to avoid an allocation)

dvdplm commented 4 months ago

Fair enough! Thank you for taking the time to hash it out. :)