-
Notifications
You must be signed in to change notification settings - Fork 103
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
Length and ScaleFactor with statically-checked units #35
Conversation
I'm confused as to why this uses phantom types. I'd think it would be simpler to just use newtype wrappers, like Au does today. |
In particular I'm a bit concerned about the complexity of the trait signatures here. |
Looks good to me. (r=me modulo @pcwalton’s concerns.) |
My earlier drafts used newtypes, but I found that it made it harder to write generic code and ended up requiring a lot more boilerplate. The current design allows all of the implementation to be generic, so adding a new unit requires only a single line of code. (See servo/servo#2444 for some examples.) I'm still pretty new to Rust, so maybe I was missing some possibilities. I'll try exploring this avenue more. One thing that the phantom types helped with was writing generic code to cast between types like Au is hard-coded to an i32 representation. This seems bad because it would lead to an explosion of newtypes like Even if we parameterize the representation (
If the concern is about readability, we can use typedefs to hide the full type signatures of commonly-used types from most users of those types. |
I'm confused…why do we want |
Possibly. DevicePixel coordinates, for example, are used as framebuffer indices, so it really makes sense for them to be We can definitely handle all of those cases by simply dropping typed units before casting to any other representation (and applying new units afterward as appropriate), though I think we can catch more errors and simplify the code if the values get tagged with units at the earliest point possible, remain tagged through any casts and calculations, and become untagged only at the last possible point before being passed to external code. |
Could we perhaps use macros to reduce the boilerplate of defining new newtype wrappers? |
To make this more concrete: When using newtypes, I had to reimplement In cases where you do want to enforce a single machine representation (and therefore don't need any type params), then with just two extra lines you can just add a public alias to a private type: enum DevicePixelPrivate {}
pub type DevicePixels = Length<DevicePixelPrivate, uint>;
pub fn DevicePixels(x: uint) -> DevicePixels { Length(x) } To the user of this code, it looks just like a newtype struct: let x: DevicePixels = DevicePixels(1);
I expect so, but I'm curious what the benefits would be versus the above. And it would still lose the ability to safely cast between representations while automatically keeping the same units, which I still believe is beneficial. |
It just feels like a lot of type-level machinery to do something simple. Long type signatures have a significant readability cost. |
What we really need is a separate units of measure library with compile-time dimensional analysis. Perhaps with macros for defining new types. I am ok with phantom types when they are used in the appropriate places, but this doesn't seem like it. |
Also adds missing `impl Zero` to make tests pass, and fixes a typo in a comment.
pub struct Length<Unit, T>(pub T); | ||
|
||
// *length | ||
impl<Unit, T> Deref<T> for Length<Unit, T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little concerned about this. This would cause unpredictable results of methods impled on T
. I would rather an explicit .get()
or .unwrap()
method to properly enforce type safety.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree; I'll change this to an explicit method. get
or unwrap
are both good names; I'm also considering val
or value
.
After re-reading this I think I am warming to the idea. |
/// let one_foot_in_mm: Length<Mm, f32> = one_foot * mm_per_inch; | ||
/// ``` | ||
#[deriving(Clone, Decodable, Encodable)] | ||
pub struct ScaleFactor<Src, Dst>(pub f32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way of making the wrapped type generic too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it would be straightforward to make this generic; the only drawback would be a bunch more type parameters throughout the implementation. I'm inclined to make this change, in order to make the rust-geom library as general as possible, though I'm also curious whether Servo could do without it. (For comparison, Gecko's ScaleFactor
uses a hard-coded single-precision float
.)
I think this is ready to land. r? @metajack |
@mbrubeck One small question, otherwise looks good. |
Length and ScaleFactor with statically-checked units
For servo/servo#2226. See code comments for details.