Skip to content

Conversation

fjarri
Copy link
Contributor

@fjarri fjarri commented Dec 17, 2023

TODO: review function names

@fjarri fjarri mentioned this pull request Dec 17, 2023
@fjarri fjarri force-pushed the primitives branch 2 times, most recently from 95960ee to 5fe3a4a Compare December 18, 2023 08:35
@tarcieri
Copy link
Member

I guess this is draft because it's WIP? Otherwise looks great so far

@fjarri
Copy link
Contributor Author

fjarri commented Dec 19, 2023

Adjusted the names and added some comments.

@fjarri fjarri marked this pull request as ready for review December 19, 2023 07:53
Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

This seems good unless you wanted to add the Hacker's Delight overflow checks and such

@fjarri
Copy link
Contributor Author

fjarri commented Dec 19, 2023

If they're used elsewhere other than const_choise.rs we can add them, but for now I think this is good enough.

@tarcieri
Copy link
Member

There is duplication between const_choice.rs and modular/boxed_residue/mul.rs

@fjarri
Copy link
Contributor Author

fjarri commented Dec 19, 2023

Not anymore.

@tarcieri
Copy link
Member

It seems like these two duplicate each other:

https://github.com/RustCrypto/crypto-bigint/blob/00e1a60/src/const_choice.rs#L71-L85

And it's also duplicated here:

https://github.com/RustCrypto/crypto-bigint/blob/00e1a60/src/modular/boxed_residue/mul.rs#L305

Potentially all three could call a common function?

@fjarri
Copy link
Contributor Author

fjarri commented Dec 19, 2023

The one in boxed_residue/mul has been removed in this PR, replaced with sbb(). The one in const_choice can indeed be expressed through the existing method, fixed. The u32/Word repetitions can be removed with a macro, but I'm not sure if it's worth it.

Copy link
Member

@tarcieri tarcieri left a comment

Choose a reason for hiding this comment

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

Nice! Looks great now

@tarcieri tarcieri merged commit c1753df into RustCrypto:master Dec 19, 2023
@fjarri fjarri deleted the primitives branch December 19, 2023 21:38
@fjarri fjarri mentioned this pull request Dec 20, 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.

2 participants