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.

Sign up for GitHub

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

Jump to bottom

add div methods on bigint #117

Merged
merged 5 commits into from
Feb 19, 2022
Merged

add div methods on bigint #117

merged 5 commits into from
Feb 19, 2022

Conversation

6293
Copy link
Collaborator

@6293 6293 commented Feb 6, 2022

@6293 6293 force-pushed the bigint-methods branch 2 times, most recently from a65f7f1 to 532350c Compare February 6, 2022 12:52
@codecov
Copy link

codecov bot commented Feb 6, 2022 •
edited
Loading

Codecov Report

Merging #117 (a4564c8) into master (5546d80) will increase coverage by 0.6%.
The diff coverage is 98.2%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    rust-amplify/rust-amplify#117     +/-   ##
========================================
+ Coverage    72.4%   73.0%   +0.6%     
========================================
  Files          33      33             
  Lines        5067    5245    +178     
========================================
+ Hits         3667    3829    +162     
- Misses       1400    1416     +16
Impacted Files Coverage Δ
num/src/lib.rs 100.0% <ø> (ø)
num/src/error.rs 22.2% <16.7%> (-2.8%) ⬇️
num/src/bigint.rs 94.0% <100.0%> (-<0.1%) ⬇️
num/src/smallint.rs 94.0% <100.0%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5546d80...a4564c8. Read the comment docs.

@6293 6293 marked this pull request as draft February 6, 2022 13:38
@6293 6293 force-pushed the bigint-methods branch 2 times, most recently from 4cf25f1 to e5bafdd Compare February 6, 2022 16:05
@6293 6293 changed the title add div methods add div methods on bigint Feb 6, 2022
@6293 6293 marked this pull request as ready for review February 7, 2022 01:34
dr-orlovsky
dr-orlovsky requested changes Feb 19, 2022
View reviewed changes
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

Two nits, the rest is looking good.

Sorry for the late review, was ill for last week+.

num/src/error.rs Show resolved Hide resolved
num/src/error.rs Show resolved Hide resolved
num/src/error.rs Show resolved Hide resolved
num/src/error.rs Show resolved Hide resolved
num/src/bigint.rs Outdated
@@ -696,7 +859,7 @@ macro_rules! construct_bigint {
type Output = $name;

fn div(self, other: T) -> $name {
self.div_rem(other.into()).0
self.div_rem(other.into()).unwrap().0
Copy link
Member

Choose a reason for hiding this comment

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

We never do unwrap in non-test code and should use expect(). Here, something like

Suggested change
self.div_rem(other.into()).unwrap().0
self.div_rem(other.into()).expect("overflow during big integer division").0

will work better

num/src/bigint.rs Outdated
@@ -716,7 +879,7 @@ macro_rules! construct_bigint {
type Output = $name;

fn rem(self, other: T) -> $name {
self.div_rem(other.into()).1
self.div_rem(other.into()).unwrap().1
Copy link
Member

Choose a reason for hiding this comment

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

Ibid

@dr-orlovsky dr-orlovsky added C-num enhancement New feature or request labels Feb 19, 2022
@dr-orlovsky dr-orlovsky added this to the v3.6 milestone Feb 19, 2022
@6293 6293 force-pushed the bigint-methods branch 2 times, most recently from d2abbc8 to bcb147d Compare February 19, 2022 08:51
@6293
Copy link
Collaborator Author

6293 commented Feb 19, 2022

Thanks for the review, unwraps are fixed to expect.

We normally use #[derive(Display)] for that

We have to support no-std in ampify-num but derive does not support no-std. Maybe that is why OverflowError in error.rs does not use Display derive?

@6293 6293 requested a review from dr-orlovsky February 19, 2022 08:56
@dr-orlovsky
Copy link
Member

You are right. Sorry, by bad

dr-orlovsky
dr-orlovsky approved these changes Feb 19, 2022
View reviewed changes
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK bcb147d

@dr-orlovsky
Copy link
Member

Seems like it needs a rebase now

@6293
Copy link
Collaborator Author

6293 commented Feb 19, 2022

rebased

dr-orlovsky
dr-orlovsky approved these changes Feb 19, 2022
View reviewed changes
Copy link
Member

@dr-orlovsky dr-orlovsky left a comment

Choose a reason for hiding this comment

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

ACK a4564c8

@dr-orlovsky dr-orlovsky merged commit 5eff8e1 into rust-amplify:master Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dr-orlovsky dr-orlovsky dr-orlovsky approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v3.6
Development

Successfully merging this pull request may close these issues.
2 participants
@6293 @dr-orlovsky