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

A few non-breaking changes #349

Merged
merged 3 commits into from Jun 29, 2019
Merged

A few non-breaking changes #349

merged 3 commits into from Jun 29, 2019

Conversation

@nical
Copy link
Collaborator

nical commented Jun 25, 2019

  • Add pre/post_transform, deprecate pre/post_mul. This name is less ambiguous: the meaning of pre and post here is really about the semantics of the tranformation and not about the underlying matrix the latter being currently in a confusing step for a number of people.
  • Add a default module with type aliases for the default unit. The goal is to rename all TypedFoo types into Foo next time we commit to a breaking change and use default::Foo for the aliases with unknown units.
  • Remove unstable must_use annotations. They add noise to the code and aren't very useful since they only work with the nightly compiler and the unstable feature.

The first two changes will let us transition some of the names at our leasure rather than having to do it all next time we do the Big Breaking Change.


This change is Reviewable

@nical
Copy link
Collaborator Author

nical commented Jun 25, 2019

r? @kvark

Note that I put default::Vector4D in lieu of default::HomogeneousVector because I prefer it that way but that's up for debate. The rest should be fairly uncontroversial I think.

@@ -220,7 +220,6 @@ where
{
/// Inflates the box by the specified sizes on each side respectively.
#[inline]
#[cfg_attr(feature = "unstable", must_use)]

This comment has been minimized.

@emilio

emilio Jun 25, 2019

Member

Servo benefits from these annotations don't they? Plus must_use is stable now, so maybe we should just remove the cfg_attr.

This comment has been minimized.

@nical

nical Jun 25, 2019

Author Collaborator

Ah nice! I hadn't realized it was stable in function annotations now. I don't know whether servo uses the unstable feature.
I'll remove the cfg_attr.

@kvark
kvark approved these changes Jun 25, 2019
Copy link
Member

kvark left a comment

Reviewed 8 of 8 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nical)


src/lib.rs, line 143 at r1 (raw file):

    use super::*;

    pub type Point2D<T> = TypedPoint2D<T, UnknownUnit>;

if we end up with just Point2D<T, U = UnknownUnit>, what would be the use for default module?

@nical
Copy link
Collaborator Author

nical commented Jun 25, 2019


src/lib.rs, line 143 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

if we end up with just Point2D<T, U = UnknownUnit>, what would be the use for default module?

These default type parameters have never ended up being useful. I'm not even sure in what situations the compiler decides to make usre of that information.

nical added 2 commits Jun 25, 2019
This is a preliminary change to start transitioning from TypedFoo names to Foo and use default::Foo for when we don't need the units (future breaking change).
@nical nical force-pushed the nical:post-transform branch from d729730 to 65c0e85 Jun 25, 2019
@nical
Copy link
Collaborator Author

nical commented Jun 29, 2019

@bors-servo r=kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jun 29, 2019

📌 Commit 65c0e85 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Jun 29, 2019

Testing commit 65c0e85 with merge b37eb9d...

bors-servo added a commit that referenced this pull request Jun 29, 2019
A few non-breaking changes

 - Add pre/post_transform, deprecate pre/post_mul. This name is less ambiguous: the meaning of pre and post here is really about the semantics of the tranformation and not about the underlying matrix the latter being currently in a confusing step for a number of people.
 - Add a `default` module with type aliases for the default unit. The goal is to rename all `TypedFoo` types into `Foo` next time we commit to a breaking change and use `default::Foo` for the aliases with unknown units.
 - Remove unstable must_use annotations. They add noise to the code and aren't very useful since they only work with the nightly compiler and the unstable feature.

The first two changes will let us transition some of the names at our leasure rather than having to do it all next time we do the Big Breaking Change.

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

bors-servo commented Jun 29, 2019

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

@bors-servo bors-servo merged commit 65c0e85 into servo:master Jun 29, 2019
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 8 files, 1 discussion left (kvark)
Details
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@nical nical deleted the nical:post-transform branch Jun 29, 2019
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.