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

Implement saturating_abs() and saturating_neg() functions for signed integer types #60192

Merged
merged 2 commits into from Apr 25, 2019

Conversation

Projects
None yet
5 participants
@t-rapp
Copy link
Contributor

commented Apr 23, 2019

Similar to wrapping_abs() / wrapping_neg() functions but saturating at the numeric bounds instead of wrapping around. Complements the existing set of functions with saturation mechanics.

cc #59983

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Apr 23, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@hellow554

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Please add some tests (especially the corner cases of course) in src/libcore/tests/num

$EndFeature, "
```"),

#[unstable(feature = "saturating_neg", issue = "0")]

This comment has been minimized.

Copy link
@hellow554

hellow554 Apr 23, 2019

Contributor

Do we want to make #59983 the tracking issue or is a seperate issue desired?

This comment has been minimized.

Copy link
@t-rapp

t-rapp Apr 23, 2019

Author Contributor

My idea was to put the issue ID of this PR therein. If using #59983 is preferred I will happily use that.

This comment has been minimized.

Copy link
@hellow554

hellow554 Apr 23, 2019

Contributor

This was more a question to the reviewer. I'm not a core member and therefore cannot decide whether to use 0 for the moment or the Issue. But a trackig issue is always an issue number, never a PR.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Thanks for the PR! I agree with @hellow554 that with some tests this is likely good to go!

For tracking issues we can either reuse the existing issue (and keep it open after this PR merges) or you can open a dedicated new issue for tracking these APIs, up to you!

@t-rapp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Have added tests. I think we can use issue #59983 for tracking. Do I need to remove the "Fixes" line in the commit message so the tracking issue does not close? And what about merging trivial commit a616f10 into fc6272a ?

@hellow554

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Do I need to remove the "Fixes" line in the commit message so the tracking issue does not close

either that or you manually reopen the issue afterwards ^^ (please somebody correct me if I'm wrong)
You can rebase your commits as you like, only merge commits are not wanted, the rest is okay-ish. As said, feel free to rebase/force push, if you want.

@t-rapp t-rapp force-pushed the t-rapp:tr-saturating-funcs branch from 6f9c903 to c3f1a3b Apr 25, 2019

@t-rapp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

It seems I somehow managed to screw up the force-push as now commit
0357e87 appears in the pull request. Sorry, I'm a newbie to GitHub.
How can I get rid of that commit on the remote branch? Would it help if I rebase to the latest upstream/master?

@hellow554

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

Go back to the commit you started by using git reset, commit the changes (you have saved before of course) and do a git push --force. Let's hope that you don't forget anything :)

t-rapp added some commits Apr 17, 2019

Add saturating_abs() and saturating_neg() functions to signed integer…
… types

Similar to wrapping_abs() / wrapping_neg() functions but saturating at
the numeric bounds instead of wrapping around. Complements the existing
set of functions with saturation mechanics.

@t-rapp t-rapp force-pushed the t-rapp:tr-saturating-funcs branch from c3f1a3b to c7d47c8 Apr 25, 2019

@t-rapp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Thanks a lot for the hints! The PR looks good now, I think.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Everything here looks good to me now, thanks again @t-rapp!

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

📌 Commit c7d47c8 has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

⌛️ Testing commit c7d47c8 with merge c32171b...

bors added a commit that referenced this pull request Apr 25, 2019

Auto merge of #60192 - t-rapp:tr-saturating-funcs, r=alexcrichton
Implement saturating_abs() and saturating_neg() functions for signed integer types

Similar to wrapping_abs() / wrapping_neg() functions but saturating at the numeric bounds instead of wrapping around. Complements the existing set of functions with saturation mechanics.

cc #59983
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing c32171b to master...

@bors bors added the merged-by-bors label Apr 25, 2019

@bors bors merged commit c7d47c8 into rust-lang:master Apr 25, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@t-rapp t-rapp deleted the t-rapp:tr-saturating-funcs branch Apr 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.