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

Make most constructors const fns #368

Merged
merged 2 commits into from Oct 4, 2019
Merged

Make most constructors const fns #368

merged 2 commits into from Oct 4, 2019

Conversation

@nical
Copy link
Collaborator

nical commented Aug 16, 2019

This will make declaring constant points, sizes, etc. quite a bit nicer.

Currently const fns only accept one trait bound (Sized), so some constructors that required Copy and other traits were moved to their own impl block to require no trait bounds.
Also Rotation2D::new(Angle<T>) could not be made const because const fns can't destroy values (even if Angle does not implement Drop). This could work if the rotation stored an Angle<T> instead of a T directly which also makes sense regardless of the appeal of const fn. It wasn't worth the breaking change but it could be something we sneak in the next breaking changes when we address the matrix conventions for example.


This change is Reviewable

@kvark
kvark approved these changes Aug 16, 2019
Copy link
Member

kvark left a comment

const for all!

@kvark
Copy link
Member

kvark commented Aug 16, 2019

Somewhat relevant - rust-gamedev/wg#28

@nical
Copy link
Collaborator Author

nical commented Aug 16, 2019

That gamedev wg thread has a point. I'm hoping that bumping to 1.31 is unlikely to break euclid users because hopefully the 2018 edition was compelling enough to have moved most of the rust ecosystem to at least that version, but I am ok with waiting for the next breaking change to land this if you want, and also ok with making it a policy to treat compiler bumps as breaking changes going forward.

@nical
Copy link
Collaborator Author

nical commented Oct 4, 2019

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented Oct 4, 2019

📌 Commit face16d has been approved by kvark

bors-servo added a commit that referenced this pull request Oct 4, 2019
Make most constructors const fns

This will make declaring constant points, sizes, etc. quite a bit nicer.

Currently const fns only accept one trait bound (Sized), so some constructors that required Copy and other traits were moved to their own `impl` block to require no trait bounds.
Also `Rotation2D::new(Angle<T>)` could not be made const because const fns can't destroy values (even if Angle does not implement Drop). This could work if the rotation stored an `Angle<T>` instead of a `T` directly which also makes sense regardless of the appeal of const fn. It wasn't worth the breaking change but it could be something we sneak in the next breaking changes when we address the matrix conventions for example.

<!-- 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/368)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Oct 4, 2019

Testing commit face16d with merge 076ac1a...

@nical
Copy link
Collaborator Author

nical commented Oct 4, 2019

Looks like proc_macro dropped support for rustc older than 1.31, without making it a breaking change, which means euclid doesn't compile on these rustcs of older vintage anymore, so lets land this now.

@bors-servo
Copy link
Contributor

bors-servo commented Oct 4, 2019

☀️ Test successful - checks-travis
Approved by: kvark
Pushing 076ac1a to master...

@bors-servo bors-servo merged commit face16d into servo:master Oct 4, 2019
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
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

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