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

Various improvements, clean up warnings, restructure constructors #81

Merged
merged 9 commits into from Jun 12, 2015

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Jun 9, 2015

  • No more warnings
  • Make most constructors into associated funtions
  • A couple other cleanup things

Basically, just check each individual commit. If not all of this is wanted, feel free to cherry pick.

@@ -35,6 +21,54 @@ pub struct Matrix4<T> {
pub m41: T, pub m42: T, pub m43: T, pub m44: T,
}

impl<T: Float> Matrix4<T> {
pub fn new(

This comment has been minimized.

@Manishearth

Manishearth Jun 9, 2015

Member

Note: We discussed removing the generics from this entirely in the meeting. But we can merge this independently.

cc @glennw

@Ms2ger
Copy link
Contributor

Ms2ger commented Jun 9, 2015

Have you updated Servo already?

@frewsxcv
Copy link
Member Author

frewsxcv commented Jun 9, 2015

Have you updated Servo already?

@Ms2ger Not yet. I wasn't sure if the changes in the pull requests were desired. If they are, I can make the changes to servo

@bors-servo
Copy link
Contributor

bors-servo commented Jun 10, 2015

The latest upstream changes (presumably #82) made this pull request unmergeable. Please resolve the merge conflicts.

@frewsxcv
Copy link
Member Author

frewsxcv commented Jun 10, 2015

From IRC:

2:34 PM <•gw> frewsxcv: sorry to get my merge in first :P personally I'm fine with that PR, it makes sense to me - but I'm not sure if there's a style guideline for using ::new() vs. what is there now - anyone?
...
3:00 PM <•jack> frewsxcv, gw: I think new is preferred. The convention (from long, long ago) was to use the type name as a function, but that is not longer the case.
...
3:02 PM <•gw> jack: thanks. frewsxcv: if you want to rebase that I'll r+ it

@frewsxcv frewsxcv force-pushed the frewsxcv:improvements branch from eeb4b99 to 5838beb Jun 11, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Jun 11, 2015

@glennw Rebased

@glennw
Copy link
Member

glennw commented Jun 11, 2015

@frewsxcv Looks good to me. Want to get the PRs done for rust-layers and servo (any anything else that needs it), and then we can merge them all at around the same time?

frewsxcv added a commit to frewsxcv/rust-layers that referenced this pull request Jun 11, 2015
frewsxcv added a commit to frewsxcv/rust-layers that referenced this pull request Jun 11, 2015
@mbrubeck
Copy link
Contributor

mbrubeck commented Jun 11, 2015

Maybe this should bump the version in Cargo.toml to 0.2.0? That might make it easier to track which downstream packages have been updated.

frewsxcv added a commit to frewsxcv/rust-geom that referenced this pull request Jun 11, 2015
@frewsxcv frewsxcv force-pushed the frewsxcv:improvements branch from c4d6dac to 27e0208 Jun 12, 2015
@frewsxcv frewsxcv force-pushed the frewsxcv:improvements branch from 27e0208 to 94a3e02 Jun 12, 2015
frewsxcv added a commit to frewsxcv/rust-offscreen-rendering-context that referenced this pull request Jun 12, 2015
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Jun 12, 2015
@pcwalton
Copy link
Contributor

pcwalton commented Jun 12, 2015

@mbrubeck Are you OK with this now? It looks fine to me.

@frewsxcv
Copy link
Member Author

frewsxcv commented Jun 12, 2015

Associated PRs:

Admittedly this does add a few characters per initialization, but I don't think it harms readability and ends up being more idiomatic.

@mbrubeck
Copy link
Contributor

mbrubeck commented Jun 12, 2015

Yes, LGTM.

frewsxcv added a commit that referenced this pull request Jun 12, 2015
Various improvements, clean up warnings, restructure constructors
@frewsxcv frewsxcv merged commit 16b91af into servo:master Jun 12, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@frewsxcv frewsxcv deleted the frewsxcv:improvements branch Jun 12, 2015
frewsxcv added a commit to frewsxcv/rust-layers that referenced this pull request Jun 12, 2015
frewsxcv added a commit to frewsxcv/rust-layers that referenced this pull request Jun 12, 2015
bors-servo pushed a commit to servo/rust-layers that referenced this pull request Jun 12, 2015
frewsxcv added a commit to frewsxcv/rust-offscreen-rendering-context that referenced this pull request Jun 12, 2015
frewsxcv added a commit to frewsxcv/rust-offscreen-rendering-context that referenced this pull request Jun 12, 2015
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Jun 12, 2015
bors-servo pushed a commit to servo/servo that referenced this pull request Jun 13, 2015
Shouldn't be merged until these have merged:

servo/euclid#81

servo/surfman#13

servo/rust-layers#178

~~I'll also need to update the Cargo lock files once they merge~~

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6349)
<!-- Reviewable:end -->
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Jun 13, 2015
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Jun 13, 2015
bors-servo pushed a commit to servo/servo that referenced this pull request Jun 13, 2015
Shouldn't be merged until these have merged:

servo/euclid#81

servo/surfman#13

servo/rust-layers#178

~~I'll also need to update the Cargo lock files once they merge~~

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6349)
<!-- Reviewable:end -->
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Jun 13, 2015
bors-servo pushed a commit to servo/servo that referenced this pull request Jun 13, 2015
Shouldn't be merged until these have merged:

servo/euclid#81

servo/surfman#13

servo/rust-layers#178

~~I'll also need to update the Cargo lock files once they merge~~

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6349)
<!-- Reviewable:end -->
frewsxcv added a commit to frewsxcv/servo that referenced this pull request Jun 13, 2015
bors-servo pushed a commit to servo/servo that referenced this pull request Jun 13, 2015
Shouldn't be merged until these have merged:

servo/euclid#81

servo/surfman#13

servo/rust-layers#178

~~I'll also need to update the Cargo lock files once they merge~~

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6349)
<!-- Reviewable:end -->
jrmuizel pushed a commit to jrmuizel/gecko-cinnabar that referenced this pull request Jun 12, 2017
…anges); r=pcwalton

Shouldn't be merged until these have merged:

servo/euclid#81

servo/surfman#13

servo/rust-layers#178

~~I'll also need to update the Cargo lock files once they merge~~

Source-Repo: https://github.com/servo/servo
Source-Revision: cfcd8589d06935f83b903f76477ea03e4d4652d0
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Sep 30, 2019
…anges); r=pcwalton

Shouldn't be merged until these have merged:

servo/euclid#81

servo/surfman#13

servo/rust-layers#178

~~I'll also need to update the Cargo lock files once they merge~~

Source-Repo: https://github.com/servo/servo
Source-Revision: cfcd8589d06935f83b903f76477ea03e4d4652d0

UltraBlame original commit: d70837d5150f00c1534f60d6ca265952259c90a3
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
…anges); r=pcwalton

Shouldn't be merged until these have merged:

servo/euclid#81

servo/surfman#13

servo/rust-layers#178

~~I'll also need to update the Cargo lock files once they merge~~

Source-Repo: https://github.com/servo/servo
Source-Revision: cfcd8589d06935f83b903f76477ea03e4d4652d0

UltraBlame original commit: d70837d5150f00c1534f60d6ca265952259c90a3
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
…anges); r=pcwalton

Shouldn't be merged until these have merged:

servo/euclid#81

servo/surfman#13

servo/rust-layers#178

~~I'll also need to update the Cargo lock files once they merge~~

Source-Repo: https://github.com/servo/servo
Source-Revision: cfcd8589d06935f83b903f76477ea03e4d4652d0

UltraBlame original commit: d70837d5150f00c1534f60d6ca265952259c90a3
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

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