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

Implement ToPrimitive and NumCast traits #58

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "app_units"
version = "0.7.6"
version = "0.7.7"
authors = ["The Servo Project Developers"]
description = "Servo app units type (Au)"
documentation = "https://docs.rs/app_units/"
Expand All @@ -13,6 +13,7 @@ num-traits = { version = "0.2", optional = true }
serde = { version = "1.0", optional = true, features = ["derive"] }

[dev-dependencies]
euclid = "0.22.11"
ron = "0.8.0"

[features]
Expand Down
110 changes: 110 additions & 0 deletions src/app_unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,54 @@ impl Au {
}
}

#[cfg(feature = "num_traits")]
// Note:
// - Conversions to isize and i128 are done implicitly based on the i64 impl
// - Conversions to other uint types are done implicitly based on the u64 impl
impl num_traits::ToPrimitive for Au {
#[inline]
fn to_f32(&self) -> Option<f32> {
Some(self.to_f32_px())
}

#[inline]
fn to_f64(&self) -> Option<f64> {
Some(self.to_f64_px())
}

#[inline]
fn to_i8(&self) -> Option<i8> {
None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not much familiar with this, but shouldn't it be self.to_px().to_i8() or such?
Analogous for all returning None.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None basically means "we don't support this cast", so it seemed like a conservative default for:

  • Unsigned integers. As casting an Au to an unsigned integer is likely a logic error.
  • Signed integers that have a size smaller than the range that Au supports. Again, we probably don't want to do this?

Taking a look at the implementations that num_traits provides for analogous stdlib types, it looks like it does runtime checks to see whether the value is in a range that can be converted.

So we could do that if you think that's better?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if it may be a weird conversion, if the value can be represented, it seems to make more sense to try to cast. But I will defer to Martin.

}

#[inline]
fn to_i16(&self) -> Option<i16> {
None
}

#[inline]
fn to_i32(&self) -> Option<i32> {
Some(self.to_px())
}

#[inline]
fn to_i64(&self) -> Option<i64> {
Some(self.to_px() as i64)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just implement to to_i64() and the others have default implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth special casing i32, because that's what we actually have? I guess casting to i64 and back again is probably pretty cheap / likely to be inlined. But still...

}

#[inline]
fn to_u64(&self) -> Option<u64> {
None
}
}

#[cfg(feature = "num_traits")]
impl num_traits::NumCast for Au {
fn from<T: num_traits::ToPrimitive>(n: T) -> Option<Self> {
n.to_f64().map(Self::from_f64_px)
}
}

#[test]
fn create() {
assert_eq!(Au::zero(), Au(0));
Expand Down Expand Up @@ -453,3 +501,65 @@ fn serialize() {
let serialized = ron::to_string(&Au(42)).unwrap();
assert_eq!(ron::from_str(&serialized), Ok(Au(42)));
}

#[cfg(feature = "num_traits")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to expand the tests a bit. There should probably be tests for the unsupported casts for instance.

#[test]
fn cast_au_u64() {
let val: Au = Au::new(120);
let cast: Option<u64> = num_traits::NumCast::from(val);
assert_eq!(cast, None)
}

#[cfg(feature = "num_traits")]
#[test]
fn cast_au_i16() {
let val: Au = Au::new(120);
let cast: Option<i16> = num_traits::NumCast::from(val);
assert_eq!(cast, None)
}

#[cfg(feature = "num_traits")]
#[test]
fn cast_au_i64() {
let val: Au = Au::new(120);
let cast: Option<i64> = num_traits::NumCast::from(val);
assert_eq!(cast, Some(2))
}

#[test]
fn cast_au_f32() {
let val: Au = Au::new(120);
let cast: Option<f32> = num_traits::NumCast::from(val);
assert_eq!(cast, Some(2.0))
}

#[cfg(feature = "num_traits")]
#[test]
fn cast_au_f64() {
let val: Au = Au::new(120);
let cast: Option<f64> = num_traits::NumCast::from(val);
assert_eq!(cast, Some(2.0))
}

#[cfg(feature = "num_traits")]
#[test]
fn euclid_cast_au_to_f32() {
use euclid::default::Size2D;
let size_au: Size2D<Au> = Size2D::new(Au::new(20 * 60), Au::new(30 * 60));
let size_f32_manual: Size2D<f32> = Size2D::new(20.0, 30.0);
let size_f32_cast: Size2D<f32> = size_au.cast();

assert_eq!(size_f32_manual, size_f32_cast);
}

#[cfg(feature = "num_traits")]
#[test]
fn euclid_cast_f32_to_au() {
use euclid::default::Size2D;
let size_f32: Size2D<f32> = Size2D::new(3.1456, 245.043656);
let size_au_manual: Size2D<Au> =
Size2D::new(Au::from_f32_px(3.1456), Au::from_f32_px(245.043656));
let size_au_cast: Size2D<Au> = size_f32.cast();

assert_eq!(size_au_manual, size_au_cast);
}
Loading