Integrate the units system proposed in issue #144 into Euclid types. #148

nical · 2016-07-26T09:41:27Z

Here is the implementation of the units system from issue #144.

Other than changing how units are expressed, I took the liberty to change the convention Typed<Unit, ScalarType> into Typed<SalarType, Unit>. This is to be able to use default generic parameters for the units. Default generics in rust are not quite where they should be yet, so the library does not take full advantage of it. When the language issues get resolved, there are some simplifications that can be made to the library (for instance not need both a TypedPoint2D and a Point2D alias).

Member access (foo.x) provides scalars (like f32) by default, and there are a few _typed convenience methods to access members as a Length (foo.x_typed()). converting rust-layers to this system, I haven't come across places where the members were accessed without unwrapping the Length right away, which tends to confirm that scalar member access should be what is convenient to write by default.

This change is

nical · 2016-07-26T17:34:54Z

r? @metajack

nical · 2016-07-27T15:42:03Z

@SimonSapin told me to r? @pcwalton instead.

pcwalton · 2016-07-27T18:02:57Z

src/matrix2d.rs

+}
+
+// Convenient aliases for TypedPoint2D with typed units
+impl<T:Copy, Src, Dst> TypedMatrix2D<T, Src, Dst> {


nit: put a space after :

pcwalton · 2016-07-27T18:04:38Z

src/matrix4d.rs

+       PartialOrd +
+       Sub<T, Output=T> +
+       Trig +
+       Zero, Src, Dst> TypedMatrix4D<T, Src, Dst> {


Please use a where clause here :)

i.e. impl<T> TypedMatrix<T, Src, Dst> where T: Add<T, Output=T> + ... {

And that reminds me, I should write an RFC to enhance Rust to allow typedefing this away. Sheesh :)

pcwalton · 2016-07-27T18:06:10Z

src/matrix4d.rs


+impl<T: Copy+fmt::Debug, Src, Dst> fmt::Debug for TypedMatrix4D<T, Src, Dst> {


uber-nit: Put spaces around +

pcwalton · 2016-07-27T19:29:15Z

src/point.rs


+// Convenient aliases for TypedPoint2D with typed units
+
+impl<T:Clone, U> TypedPoint2D<T, U> {


nit: space after :

pcwalton · 2016-07-27T19:35:44Z

src/rect.rs

    }
 }

-impl<T: Copy + Clone + PartialOrd + Add<T, Output=T> + Sub<T, Output=T> + Zero> Rect<T> {
+impl<T: Copy + Clone + PartialOrd + Add<T, Output=T> + Sub<T, Output=T> + Zero, U> TypedRect<T, U> {


While you're here, could you change this to use a where clause?

pcwalton · 2016-07-27T19:36:46Z

src/side_offsets.rs

 use num::Zero;
+use length::{ Length, Untyped };


nit: no spaces after { and before }

pcwalton · 2016-07-27T19:37:15Z

src/size.rs

@@ -7,117 +7,158 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

-use length::Length;
+use length::{ Length, Untyped };
+use scale_factor::{ ScaleFactor };


nit: no spaces after { and before }, and also no need to brace {ScaleFactor}

nical · 2016-07-28T07:58:40Z

Thanks for the quick review! I addressed all of the review items and also changed "Untyped" into "UnknownUnit" Sorry I did that so late in the review process. I just realized gecko had renamed its default unit this way a while ago and the name is more understandable I think.

pcwalton · 2016-07-28T17:53:23Z

Looks good! Squash the commits together if you'd like and I'll r=me.

To anyone interested: If there are any comments about this change, speak now or forever hold your peace :)


        Integrate the new units system into Euclid types.

nical · 2016-07-29T10:54:45Z

Commits squashed!

pcwalton · 2016-07-29T16:27:09Z

@bors-servo: r+

bors-servo · 2016-07-29T16:27:11Z

📌 Commit 6205104 has been approved by pcwalton

bors-servo · 2016-07-29T16:27:13Z

⌛ Testing commit 6205104 with merge 547b5c3...


        Auto merge of #148 - nical:units, r=pcwalton

Integrate the units system proposed in issue #144 into Euclid types. Here is the implementation of the units system from issue #144. Other than changing how units are expressed, I took the liberty to change the convention ```Typed<Unit, ScalarType>``` into ```Typed<SalarType, Unit>```. This is to be able to use default generic parameters for the units. Default generics in rust are not quite where they should be yet, so the library does not take full advantage of it. When the language issues get resolved, there are some simplifications that can be made to the library (for instance not need both a TypedPoint2D and a Point2D alias). Member access (```foo.x```) provides scalars (like f32) by default, and there are a few ```_typed``` convenience methods to access members as a Length (```foo.x_typed()```). converting rust-layers to this system, I haven't come across places where the members were accessed without unwrapping the Length right away, which tends to confirm that scalar member access should be what is convenient to write by default.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/148)

bors-servo · 2016-07-29T16:30:14Z

☀️ Test successful - travis

peterjoel · 2016-08-13T17:44:05Z

Euclid is already at 0.9. This will need to be incremented before merging.

nical force-pushed the nical:units branch 2 times, most recently from 9f729fc to 3178c36 Jul 26, 2016

nical force-pushed the nical:units branch from 3178c36 to 066e83b Jul 27, 2016