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

Add saturating_mul() and refactor Saturating into subtraits. Fixes #40. #165

Merged
merged 4 commits into from
Jun 11, 2020

Conversation

trepetti
Copy link
Contributor

Hi all,

Taking the suggestion from #40, this pull request breaks out the saturating trait into three separate SaturatingAdd, SaturatingSub and SaturatingMul traits, borrowing the idea from Checked* and Wrapping*. In terms of native ops, SaturatingNeg would be an option on signed types once the saturating_neg() API is stable on signed integer primitives.

I know there is concern about breaking changes, so I was thinking that I could add a wrapping Saturating trait for backwards compatibility that retains the original functionality:

pub trait Saturating: SaturatingAdd + SaturatingSub {}
impl<T> Saturating for T where T: SaturatingAdd + SaturatingSub {}

This is not included in the current version, but I would be happy to add.

-Tom (trepetti@cs.columbia.edu)

@trepetti
Copy link
Contributor Author

trepetti commented May 1, 2020

Also, it occurred to me that there is no harm in making a backwards compatible compound Saturating trait from SaturatingAdd + SaturatingSub + SaturatingMul since there are no primitive integer types that support the first two without SaturatingMul. So that may be an option, too.

@cuviper
Copy link
Member

cuviper commented May 2, 2020

I know there is concern about breaking changes, so I was thinking that I could add a wrapping Saturating trait for backwards compatibility that retains the original functionality:

pub trait Saturating: SaturatingAdd + SaturatingSub {}
impl<T> Saturating for T where T: SaturatingAdd + SaturatingSub {}

Breaking changes are definitely a concern, and unfortunately this idea doesn't solve it. That would be fine for users of the trait, but it still breaks any implementors of the trait. Since the trait is not sealed, 3rd parties can and do implement it as well, e.g. extprim::u128. That concern applies to the constraint change on PrimInt too.

We can instead deprecate Saturating, nudging folks to use the new separate traits. PrimInt will be stuck with the old one, but that's life.

@trepetti
Copy link
Contributor Author

trepetti commented May 2, 2020

Good point! Sorry I did not think that through entirely.

I restored the original Saturating trait and updated the doc comment to mention it has been deprecated. Similarly, I restored PrimInt to its original state with just Saturating and now expose all four Saturating, SaturatingAdd, SaturatingSub and SaturatingMul in lib.rs.

Let me know if there is a better way to go about deprecating Saturating, if that is the route you want to take. Looking back, it seemed that just mentioning as much in the doc comments is what has been done historically.

Thank you for looking at my pull request.

}
};
($trait_name:ident, $method:ident, $t:ty, $rhs:ty) => {
impl $trait_name<$rhs> for $t {
Copy link
Member

Choose a reason for hiding this comment

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

Is this second macro pattern actually used? I think not, because your new traits don't have a generic RHS.

@cuviper cuviper mentioned this pull request Jun 1, 2020
@cuviper
Copy link
Member

cuviper commented Jun 11, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 11, 2020

@bors bors bot merged commit 9d54f39 into rust-num:master Jun 11, 2020
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.

None yet

2 participants