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

Saturating<T> wrapper type #175

Closed
wants to merge 13 commits into from
Closed

Saturating<T> wrapper type #175

wants to merge 13 commits into from

Conversation

m4rw3r
Copy link

@m4rw3r m4rw3r commented Mar 4, 2016

Rust's stdlib has a Wrapping<T> wrapper type for primitive integer types. The type provides intentionally wrapping semantics to all operations performed on it.

This pull-request introduces a Saturating<T> wrapper type which is the corresponding saturating wrapper type for primitive integer types. This type provides saturating semantics on arithmetic and bit-shifting operations for both signed and unsigned integers.

The goal is to provide a type which is easy to slot in instead of either Wrapping<T> or any basic integer type. Combined with the traits in num it would be possible to make functions/methods generic over the type of integer and if it should have wrapping or saturating semantics.

The name Saturating<T> collides with the trait Saturating which is unfortunate. I suspect that this type will need a name change but at the moment I do not have any suggestions.


#[inline(always)]
fn max_value() -> Self {
Saturating(<$t>::min_value())
Copy link

Choose a reason for hiding this comment

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

Uh this doesn't look right...

Copy link
Author

Choose a reason for hiding this comment

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

Oops

@cuviper
Copy link
Member

cuviper commented Mar 5, 2016

Saturating<T> was also just suggested for the standard library in rust-lang/rfcs#1218 (comment). It's a reasonable idea, but don't you think it would make more sense implemented next to Wrapping<T>?

I'm not sure that I agree that implementing num traits for this is helpful. Notice we have nothing yet implemented for Wrapping<T> either. I feel like it would be surprising for a generic function taking Num to encounter wrapping or saturating semantics. Can you give a concrete example where this would really be useful to you?

@m4rw3r
Copy link
Author

m4rw3r commented Mar 5, 2016

@cuviper I guess it depends on the number of people who think it would be useful, but it does make more sense to provide both Wrapping<T>, Saturating<T> and maybe even some Checked<T> (always panic on overflow) in the standard library.

As for usefulness in the generic case I have some code which is read from external input where I do not mind if the values are saturated if they are out of bounds, here it is useful with a saturating datatype since I do not have to manually perform checks to make sure that the data I read is within the range of the datatype.

@SoniEx2
Copy link

SoniEx2 commented Mar 5, 2016

Would be nice to have traits for wrapping_add(), saturating_add(), etc, and have a Wrapping, Saturating, etc impl for them. But that's another issue.

@cuviper
Copy link
Member

cuviper commented Mar 6, 2016

@m4rw3r then will you propose this for the standard library instead?
If that's rejected, then we can reconsider it here...

@m4rw3r
Copy link
Author

m4rw3r commented Mar 7, 2016

@cuviper Submitted as an RFC here: rust-lang/rfcs#1534 this also includes the always-checked counterpart too.

@homu
Copy link
Contributor

homu commented Apr 13, 2016

☔ The latest upstream changes (presumably #164) made this pull request unmergeable. Please resolve the merge conflicts.

@cuviper
Copy link
Member

cuviper commented Dec 19, 2017

If this is ever revived, it will need to be on the separate repo:
/~https://github.com/rust-num/num-traits

@cuviper cuviper closed this Dec 19, 2017
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.

5 participants