Conversation
src/system.rs
Outdated
| } | ||
|
|
||
| #[inline(always)] | ||
| fn clamp(self, min: Self, max: Self) -> Self { |
There was a problem hiding this comment.
This implementation isn't explicitly needed. Ord has a default implementation. I supose it's possible a manual implementation could be performant.
src/system.rs
Outdated
| pub fn clamp(self, min: Self, max: Self) -> Self | ||
| where | ||
| V: $crate::num::Float, | ||
| { | ||
| Quantity { | ||
| dimension: $crate::lib::marker::PhantomData, | ||
| units: $crate::lib::marker::PhantomData, | ||
| value: self.value.clamp(min.value, max.value), |
There was a problem hiding this comment.
This is failing because it doesn't know where the value.clamp function is defined for the generic type V. The compiler is guessing the Ord trait because it is the only one with a clamp method. I see a couple solutions:
- Use https://docs.rs/num/latest/num/fn.clamp.html. This may not be as efficient for
f32/f64. - Create another
storage_types!block just for float and then the code will compile
There was a problem hiding this comment.
If you're going to raise the MSRV to 1.50.0, then I'd rather couple to the standard behaviour.
Create another
storage_types!block just for float
You mean, alongside the one which already exists for Float and Complex?
If so, I'm getting something that compiles, but fails tests because of panics in the case that max < min. How do you put constraints on what quickcheck generates?
There was a problem hiding this comment.
Both my suggestions were in the context of raising the MSRV to 1.50. The first, using num::clamp, could be implemented in the impl<D, U, V> Quantity<D, U, V> ... block and would apply to all underlying storage types, however because it's using num::clamp it may not be as performant as a direct f32::clamp call. The second suggestion would be another storage_types! block that is just for float (the existing can't be used because Complex doesn't have a clamp method) and would essentially proxy f32/f64 clamp. I lean towards the second option.
Reject invalid test input using TestResult::discard();:
Lines 112 to 114 in 7c4b27b
There was a problem hiding this comment.
Should I add some #[should_panic] tests? If so, I'm not sure how these fit in to the test strucre.
There was a problem hiding this comment.
Sure, something like ??? with #[should_panic] can be added as a non-quickcheck test up around line 263.
|
@iliekturtles Is there any way I can help get this merged? The last failure seems to be about old MSRV |
@dnsco, @jacg I took a brief look and it seems like rebasing+squashing on master fixes the previous build failures. |
|
I'm looking forward to this feature! :) |
4 participants
clamphas two distinct implementations, one inOrd(which floats aren't) and another inf{32,64}.I don't know how to separate these inside
uom.