Skip to content

Make impl_modulus! usable from external crates#172

Merged
tarcieri merged 1 commit into
RustCrypto:masterfrom
haslersn:impl-modulus-ext
Jan 23, 2023
Merged

Make impl_modulus! usable from external crates#172
tarcieri merged 1 commit into
RustCrypto:masterfrom
haslersn:impl-modulus-ext

Conversation

@haslersn

Copy link
Copy Markdown
Contributor

Previously it was impossible to use impl_modulus! in crates that use crypto-bigint, because certain functions were only available within the crypto-bigint crate.

@haslersn haslersn force-pushed the impl-modulus-ext branch 2 times, most recently from df75075 to 26de13e Compare January 23, 2023 13:39
Comment thread src/uint/div.rs Outdated
Comment thread src/uint/modular.rs Outdated

@mikelodder7 mikelodder7 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Makes sense. Nothing else better we could do than define macros to create constants?

@tarcieri

Copy link
Copy Markdown
Member

Not for now. When const generics improve we can get rid of the macros.

@fjarri

fjarri commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

I haven't been wild about their name. They're really "const_" as opposed to "ct_", and the wrapper functions which return subtle-based types like CtOption and Choice are also constant-time, which makes the ct_ prefix confusing in the public API.

I would vote for making the const fns have const_ prefix. They won't intersect with the subtle-based functions since those can't be used in the const context anyway. We don't need a special ct_ prefix for constant-timeness since the library already postulates that it's constant-time by default, and anything not constant-time is specifically marked as _vartime. The only problem are the const fn analogues of subtle methods which have ct_ prefixes for consistency reasons, but I think they're all currently crate-private.

Edit: on a second thought, in many cases we don't even need the const_ prefix since there are no non-const counterparts (e.g. bits()).

@tarcieri

Copy link
Copy Markdown
Member

@haslersn can you add a test to the tests/ directory to ensure that impl_modulus! works via the crate's public API?

Otherwise I'd say this is fine and we can circle back on the ct_ prefix separately.

@haslersn haslersn force-pushed the impl-modulus-ext branch 2 times, most recently from 4c19d46 to caed55f Compare January 23, 2023 17:10
@haslersn

Copy link
Copy Markdown
Contributor Author

Should now be all done including the const_ prefix:

  • Added test
  • Added $crate qualification in the impl_modulus! implementation so that the macro's user doesn't need additional imports
  • Renamed ct_rem{_wide} to const_rem{_wide}

@tarcieri

Copy link
Copy Markdown
Member

@haslersn hmm, I don't see the test?

Previously it was impossible to use `impl_modulus!` in crates that
use crypto-bigint, because certain functions were only available
within the crypto-bigint crate.
@haslersn

Copy link
Copy Markdown
Contributor Author

I don't see the test

Oops, git add -p doesn't cover new files.

@tarcieri

Copy link
Copy Markdown
Member

@haslersn think you still need to push the changes

@haslersn

Copy link
Copy Markdown
Contributor Author

No, they were pushed just before my previous comment

@tarcieri

Copy link
Copy Markdown
Member

Oh sorry, didn’t refresh

@tarcieri tarcieri merged commit b754b2a into RustCrypto:master Jan 23, 2023
@haslersn haslersn deleted the impl-modulus-ext branch January 23, 2023 20:00
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.

4 participants