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

Signed integers #1718

Merged
merged 33 commits into from
Jun 30, 2023
Merged

Signed integers #1718

merged 33 commits into from
Jun 30, 2023

Conversation

chipshort
Copy link
Collaborator

@chipshort chipshort commented Jun 9, 2023

closes #1710

  • reimplemented uints using bnum crate
  • started adding ints (also based on bnum)
  • had to update CI rust version to 1.65 for it to compile
  • commented out tarpaulin in CI for now, because the version we use does not use rust 1.65, but we might not be able to update easily (see Upgrade tarpaulin to 0.22.0+ #1455)

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

🔥

packages/std/src/errors/std_error.rs Outdated Show resolved Hide resolved
packages/std/src/math/int512.rs Show resolved Hide resolved
packages/std/src/math/int512.rs Outdated Show resolved Hide resolved
packages/std/src/math/int512.rs Show resolved Hide resolved
packages/std/src/math/int512.rs Outdated Show resolved Hide resolved
packages/std/src/math/int512.rs Outdated Show resolved Hide resolved
packages/std/src/math/int512.rs Show resolved Hide resolved
packages/std/src/math/int512.rs Show resolved Hide resolved
packages/std/src/math/uint256.rs Show resolved Hide resolved
@chipshort chipshort changed the base branch from main to release/1.3 June 28, 2023 16:14
@chipshort chipshort marked this pull request as ready for review June 28, 2023 16:14
@chipshort
Copy link
Collaborator Author

I added the missing signed ints and rebased on 1.3

packages/std/Cargo.toml Outdated Show resolved Hide resolved
packages/std/src/math/int128.rs Outdated Show resolved Hide resolved
packages/std/src/math/int128.rs Outdated Show resolved Hide resolved
packages/std/src/math/int256.rs Outdated Show resolved Hide resolved
packages/std/src/math/int256.rs Outdated Show resolved Hide resolved
packages/std/src/math/int64.rs Outdated Show resolved Hide resolved
packages/std/src/math/mod.rs Show resolved Hide resolved
Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Very very nice stuff 🙌 Let's add a CHANGELOG entry and merge

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Partial review up to int64 (without tests) inclusive.

Looks good. Will review the other int types later. And take a look at the tests.

packages/std/src/math/uint512.rs Show resolved Hide resolved
packages/std/src/math/int64.rs Show resolved Hide resolved
packages/std/src/math/int64.rs Show resolved Hide resolved
/// This operation will panic if `rhs` is zero.
#[inline]
fn rem(self, rhs: Self) -> Self {
Self(self.0.rem(rhs.0))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Rely on checked_rem for consistency?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to keep it consistent with the unsigned ones (they have the same implementation for rem).

Actually, I think it might be better to change the ones using self.0.checked_* to use either self.0.*, a self.checked_* call or an explicit message using .expect. That would give more helpful panic messages.
But I think we can do that later.

}
forward_ref_op_assign!(impl ShlAssign, shl_assign for Int64, u32);

impl Serialize for Int64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps not worth it, but, move all the serialisations to a int_se module?

}
}

impl<'de> Deserialize<'de> for Int64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, own int_de module? Not sure about this, just saying.

Self: Add<A, Output = Self>,
{
fn sum<I: Iterator<Item = A>>(iter: I) -> Self {
iter.fold(Self::zero(), Add::add)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

Copy link
Contributor

@hashedone hashedone left a comment

Choose a reason for hiding this comment

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

LGTM, I hope I didn't miss any typos in indexing sizes, but I don't see any. I am wondering if we could just reexport bnum types. However, I see we do error alignment in checked functions, so maybe it is not the best idea.

@chipshort chipshort merged commit cad5513 into release/1.3 Jun 30, 2023
@chipshort chipshort deleted the chipshort/signed-math branch June 30, 2023 13:23
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.

Signed integers: Int64, Int128, Int256, Int512
4 participants