Skip to content

Implement Div for Wrapping<T> #26885

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

Merged
merged 2 commits into from
Jul 9, 2015
Merged

Implement Div for Wrapping<T> #26885

merged 2 commits into from
Jul 9, 2015

Conversation

mvdnes
Copy link
Contributor

@mvdnes mvdnes commented Jul 8, 2015

Resolves #26867

@rust-highfive
Copy link
Contributor

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@mvdnes
Copy link
Contributor Author

mvdnes commented Jul 8, 2015

Ah hmm I seem to have misunderstood the stability attribute.
Should I remove it, rename the feature, or something else?


#[inline(always)]
fn div(self, other: Wrapping<$t>) -> Wrapping<$t> {
Wrapping(self.0 / other.0)
Copy link
Member

Choose a reason for hiding this comment

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

This should look like Wrapping(self.0.wrapping_div(other.0)); currently your code will panic rather than overflow on MIN/-1 ∀ signed integers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, I hadn't looked properly.

@nagisa
Copy link
Member

nagisa commented Jul 8, 2015

Just call feature wrapping_div or something.

@alexcrichton
Copy link
Member

@bors: r+ a89b3ea

Thanks!

bors added a commit that referenced this pull request Jul 9, 2015
@bors
Copy link
Collaborator

bors commented Jul 9, 2015

⌛ Testing commit a89b3ea with merge 9c3ba76...

@bors bors merged commit a89b3ea into rust-lang:master Jul 9, 2015
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.

6 participants