Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRemove some ApproxEq bounds #259
Merged
Conversation
|
Thanks a lot. I am 300% for the removal of the check in normalize. Could you bump the version as well, so that we can publish this right away? |
|
Great! I bumped the version to 0.16.2. |
|
@bors-servo r+ |
|
|
bors-servo
added a commit
that referenced
this pull request
Jan 13, 2018
Remove some ApproxEq bounds This PR removes `ApproxEq` bounds from several functions. In most cases the bound was unneeded, the major exception being `normalize()`. Here, I removed the divide by zero checks. Let me argue why this is reasonable. - The previous behavior was surprising. Similar crates (e.g. cgmath, nalgebra) do not make checks. (nalgebra has an additional `try_normalize()` that takes a `min_norm` argument.) - It was inconsistent with other similar functions in euclid that either do not make checks or return an `Option`. Also, the latter is only done for exact equality with zero and not approximate equality. - It caused a bug in one of my programs.😄 <!-- 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/259) <!-- Reviewable:end -->
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
pizzaiter commentedJan 13, 2018
•
edited by larsbergstrom
This PR removes
ApproxEqbounds from several functions. In most cases the bound was unneeded, the major exception beingnormalize(). Here, I removed the divide by zero checks. Let me argue why this is reasonable.try_normalize()that takes amin_normargument.)Option. Also, the latter is only done for exact equality with zero and not approximate equality.This change is