Skip to content

(breaking change) Don't implement Floor, Ceil and Round on integer types. #449

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

Merged
merged 1 commit into from
Jul 10, 2020

Conversation

jrmuizel
Copy link
Contributor

@jrmuizel jrmuizel commented Jul 9, 2020

I can't see a reason to have it.

@nical nical changed the title Don't implement Floor, Ceil and Round on integer types. (breaking change) Don't implement Floor, Ceil and Round on integer types. Jul 9, 2020
@nical
Copy link
Contributor

nical commented Jul 10, 2020

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 609955f has been approved by nical

@bors-servo
Copy link
Contributor

⌛ Testing commit 609955f with merge 508cbce...

@bors-servo
Copy link
Contributor

☀️ Test successful - checks-travis
Approved by: nical
Pushing 508cbce to master...

@bors-servo bors-servo merged commit 508cbce into servo:master Jul 10, 2020
@kyren
Copy link
Contributor

kyren commented Aug 7, 2020

I actually have a use case for exactly this and discovered this issue when fixing my build after upgrading to euclid 0.22. Having these implementations allows me to treat f32, i32 the same in a generic context where getting the i32 bounding box is important.

It's not critical for me because I can work around the problem basically by making my own trait for it with a round_out method that is a no-op for integers, but it's slightly annoying.

If it would be welcome, I'd make a PR to add this back, but I understand if it's just not an API surface you want in the future.

@nical
Copy link
Contributor

nical commented Aug 7, 2020

I'm fine with adding it back.

@kyren
Copy link
Contributor

kyren commented Aug 20, 2020

I ended up needing it twice, so I figure that passes the bar of being useful for me at least, so I made a PR: #464

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants