Skip to content

Num traits#218

Closed
ycscaly wants to merge 14 commits into
RustCrypto:masterfrom
ycscaly:num_traits
Closed

Num traits#218
ycscaly wants to merge 14 commits into
RustCrypto:masterfrom
ycscaly:num_traits

Conversation

@ycscaly

@ycscaly ycscaly commented Apr 30, 2023

Copy link
Copy Markdown
Contributor

Resolves #158

Comment thread src/wrapping.rs
fn num_traits() {
const TEST: Wrapping<U64> = Wrapping(U64::from_u64(0x0011223344556677));

assures_num(TEST)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My logic here is: Num is implemented for Wrapping<T> where ... so that we're implementing it for the minimally restricted type, but we want to assure that it works for Wrapping<Uint<Limbs>>, so this code assures it.

I'm not sure if there's a better way of checking this.

Comment thread src/wrapping.rs
{
type FromStrRadixErr = ();

fn from_str_radix(str: &str, radix: u32) -> Result<Self, Self::FromStrRadixErr> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How do we want to handle this?

Obviously this is unimplemented logic for this crate, and I'm not sure whether we want to support this (or, for example, return an error for any radix other than 2, 10 or 16.)

num-bigint's implementation is complex, but I could copy it if that's the way to go.

My thoughts are to actually to use from_str_radix() implementation of u32 and u64, that is, to parse limb-by-limb and sum them up.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

#246 and/or #247 are some options, although I took issue with them not being constant-time

Comment thread src/wrapping.rs
{
type Output = Wrapping<T>;

fn div(self, rhs: Wrapping<T>) -> Self::Output {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This and rem panics in case of division by zero.
This is the intended behavior of this crate, but I'm not sure that "this is the right panic" - i.e. now we'll panic on unwrap()ping of NonZero on zero, and the panic message might not be ideal; should I instead assert_ne(rhs, T::Zero) instead?

@ycscaly ycscaly marked this pull request as ready for review April 30, 2023 10:02
@ycscaly ycscaly marked this pull request as draft April 30, 2023 10:02
@ycscaly

ycscaly commented Apr 30, 2023

Copy link
Copy Markdown
Contributor Author

@tarcieri please see and respond to my comments so I can proceed

Comment thread src/wrapping.rs
@tarcieri

Copy link
Copy Markdown
Member

#433 covers quite a bit of this

@tarcieri

tarcieri commented May 7, 2024

Copy link
Copy Markdown
Member

Closing in favor of #433. Please open a new PR if you would like additional support which wasn't covered there.

@tarcieri tarcieri closed this May 7, 2024
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.

num-traits support (optional)

2 participants