Implement `ToPrimitive` and `NumCast` traits #58

nicoburns · 2024-12-12T00:39:38Z

This hooks Au into euclids type casting mechanism and will allow us to convert code like:

let new_viewport_size = Size2D::new(
    Au::from_f32_px(window_size_data.initial_viewport.width),
    Au::from_f32_px(window_size_data.initial_viewport.height),
);

into

let new_viewport_size = window_size_data.initial_viewport.cast::<Au>();

Signed-off-by: Nico Burns <[email protected]>

Loirooriol · 2024-12-12T01:36:35Z

src/app_unit.rs

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


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

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?

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.

mrobinson

Interesting! I don't have a strong opinion about whether or not we should be conservative or liberal in what kind of casts we allow. Is it possible to make the cast conservative now and then make it more liberal in the future though, by no longer returning None? If that's the case maybe it's better to not do something that is hard to undo and stay conservative for the time being.

mrobinson · 2024-12-12T18:16:03Z

src/app_unit.rs

+    #[inline]
+    fn to_u8(&self) -> Option<u8> {
+        None
+    }
+
+    #[inline]
+    fn to_u16(&self) -> Option<u16> {
+        None
+    }
+
+    #[inline]
+    fn to_u32(&self) -> Option<u32> {
+        None
+    }
+
+    #[inline]
+    fn to_u64(&self) -> Option<u64> {
+        None
+    }
+
+    #[inline]
+    fn to_u128(&self) -> Option<u128> {
+        None
+    }
+
+    #[inline]
+    fn to_usize(&self) -> Option<usize> {
+        None
+    }


It looks like you could simply implement to_u64 and then the other implementations have defaults?

Yes, I could do. Did it this way to be explicit. But I can go down that route if you prefer?

mrobinson · 2024-12-12T18:16:50Z

src/app_unit.rs

@@ -453,3 +539,27 @@ fn serialize() {
    let serialized = ron::to_string(&Au(42)).unwrap();
    assert_eq!(ron::from_str(&serialized), Ok(Au(42)));
 }
+
+#[cfg(feature = "num_traits")]


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

mrobinson · 2024-12-12T18:19:00Z

src/app_unit.rs

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


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

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...

mrobinson · 2024-12-12T18:19:43Z

src/app_unit.rs

@@ -289,6 +289,92 @@ impl Au {
    }
 }

+#[cfg(feature = "num_traits")]
+impl num_traits::ToPrimitive for Au {
+    // Float conversions


I think these comments aren't adding too much. It's pretty obvious what the conversion is by looking at the type signature.

Loirooriol · 2024-12-12T18:37:27Z

@emilio had some doubts about this in #46

nicoburns · 2024-12-12T19:51:36Z

Interesting! I don't have a strong opinion about whether or not we should be conservative or liberal in what kind of casts we allow. Is it possible to make the cast conservative now and then make it more liberal in the future though, by no longer returning None? If that's the case maybe it's better to not do something that is hard to undo and stay conservative for the time being.

Yes. I believe so. And this was part of my reasoning for doing it like this.

The effect of doing that is that initially (when None is being called) calls to try_cast will return None and calls to cast will panic (so these casts will likely not be used). And then if the implementation is changed then those calls will start succeeding.

nicoburns · 2024-12-12T20:43:28Z

@emilio had some doubts about this in #46

I somewhat agree that this could be confusing. I think this is Euclid's fault for having "cast" as it's name/mechanism for doing this. We could potentially add a different method/mechanism. Taffy's "geom" containers have map methods which would allow us to do things like size.map(Au::from_f32_px). Which is not quite as short as casting, but it's more explicit.

Although IMO Au::new is worse in this regard. I would expect Au::new(120).as_f32_px() to be 120.0 not 2.0.

Signed-off-by: Nico Burns <[email protected]>

nicoburns added the enhancement label Dec 12, 2024

nicoburns requested review from mrobinson and Loirooriol December 12, 2024 00:39

nicoburns force-pushed the casting-traits branch from 7eed210 to bb2bfdf Compare December 12, 2024 00:47

nicoburns added 3 commits December 12, 2024 13:55

Implement ToPrimitive and NumCast traits

0dc0d49

Signed-off-by: Nico Burns <[email protected]>

Bump version to 0.7.7

7ce8644

Signed-off-by: Nico Burns <[email protected]>

Add test for euclid casting

aa000a2

Signed-off-by: Nico Burns <[email protected]>

nicoburns force-pushed the casting-traits branch from bb2bfdf to aa000a2 Compare December 12, 2024 00:55

Loirooriol reviewed Dec 12, 2024

View reviewed changes

mrobinson requested changes Dec 12, 2024

View reviewed changes

nicoburns added 2 commits December 13, 2024 12:56

Remove unnecessary functions from ToPrimitive impl

4891241

Add more tests

77b4a6c

Signed-off-by: Nico Burns <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `ToPrimitive` and `NumCast` traits #58

Implement `ToPrimitive` and `NumCast` traits #58

nicoburns commented Dec 12, 2024

Loirooriol Dec 12, 2024

nicoburns Dec 12, 2024

Loirooriol Dec 12, 2024

mrobinson left a comment

mrobinson Dec 12, 2024

nicoburns Dec 12, 2024

mrobinson Dec 12, 2024

mrobinson Dec 12, 2024

nicoburns Dec 12, 2024

mrobinson Dec 12, 2024

Loirooriol commented Dec 12, 2024

nicoburns commented Dec 12, 2024

nicoburns commented Dec 12, 2024

Implement ToPrimitive and NumCast traits #58

Are you sure you want to change the base?

Implement ToPrimitive and NumCast traits #58

Conversation

nicoburns commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrobinson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Loirooriol commented Dec 12, 2024

nicoburns commented Dec 12, 2024

nicoburns commented Dec 12, 2024

Implement `ToPrimitive` and `NumCast` traits #58

Implement `ToPrimitive` and `NumCast` traits #58