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 support for Saturating add and subtract #168

Merged
merged 2 commits into from Nov 18, 2016

Conversation

@absoludity
Copy link
Contributor

absoludity commented Oct 31, 2016

Adds support for Length.saturating_add and saturating_sub for #103

Let me know if the intention is to support Saturating for other types also (happy to do separate branches).


This change is Reviewable

@absoludity absoludity changed the title Add support for Saturating add and subtract #103 Add support for Saturating add and subtract Oct 31, 2016
@jdm
Copy link
Member

jdm commented Nov 1, 2016

r? @glennw

@emilio
emilio approved these changes Nov 18, 2016
Copy link
Member

emilio left a comment

Looks good to me without that comment, thanks for doing this! And sorry it stayed here so long without a review :(

@@ -117,6 +117,18 @@ impl<U, T: Clone + SubAssign<T>> SubAssign for Length<T, U> {
}
}

// Saturating length + length and length - length.
impl<U, T: Clone + Saturating> Saturating for Length<T, U> {
// type Output = Length<T, U>;

This comment has been minimized.

@emilio

emilio Nov 18, 2016

Member

Remove the comment please.

@absoludity
Copy link
Contributor Author

absoludity commented Nov 18, 2016

@emilio np, thanks for taking the time to review :). I removed the commented out Output type... why is it not required/desired in this function but is throughout all the others in length.rs?

@emilio
Copy link
Member

emilio commented Nov 18, 2016

It's not required because it's not part of the trait at https://github.com/rust-num/num/blob/master/traits/src/ops/saturating.rs.

I don't think a saturating operation makes a lot of sense if the output can have a different type (and thus range), so I think it makes sense to require that the output of the function is the same type in this case.

@bors-servo r+

If it's urgent or you need it somewhere I can PR a version bump and publish it, otherwise it'll be published with the next version bump.

Thanks a bunch!

@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2016

📌 Commit 72e9ac9 has been approved by emilio

@bors-servo
Copy link
Contributor

bors-servo commented Nov 18, 2016

Test exempted - status

@bors-servo bors-servo merged commit 72e9ac9 into servo:master Nov 18, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test exempted
Details
bors-servo added a commit that referenced this pull request Nov 18, 2016
Add support for Saturating add and subtract

Adds support for Length.saturating_add and saturating_sub for #103

Let me know if the intention is to support Saturating for other types also (happy to do separate branches).

<!-- Reviewable:start -->

---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/168)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.