Conversation
There was a problem hiding this comment.
Pull request overview
Updates ordering behavior for AssetCapacity comparisons, specifically for continuous capacities, to change how floating-point values are compared.
Changes:
- Replaced
total_cmpwithpartial_cmp(...).expect(...)forAssetCapacity::ContinuousinsideOrd::cmp.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hmm... I can make this tidier, actually. Lemme have another go. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1207 +/- ##
==========================================
+ Coverage 89.02% 89.29% +0.26%
==========================================
Files 57 57
Lines 7838 7976 +138
Branches 7838 7976 +138
==========================================
+ Hits 6978 7122 +144
+ Misses 564 557 -7
- Partials 296 297 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
alexdewar
force-pushed
the
fix-assetcapacity-semantics
branch
from
15db796 to
6269b0f
Compare
…pacity` The implementations for these traits shouldn't panic and can cause UB if they do. The only thing we need them for is the `min()` method, but we can just implement this manually instead. Fixes #1206.
alexdewar
force-pushed
the
fix-assetcapacity-semantics
branch
from
6269b0f to
f60f62a
Compare
|
I've had another go. I ended up going down a bit of a rabbit hole with this... it actually seems that having implementations of As we only only use |
There was a problem hiding this comment.
Pull request overview
Updates AssetCapacity comparisons to use a true partial ordering (returning None when capacities aren’t meaningfully comparable) and adds an explicit AssetCapacity::min helper that preserves the prior .min(...) call sites without relying on Ord.
Changes:
- Added
AssetCapacity::minthat panics when a comparison is not meaningful. - Replaced the previous total ordering (
Eq/Ordwithtotal_cmp/panics) with aPartialOrdimplementation that returnsNonefor incomparable cases. - Adjusted discrete comparisons to only compare unit counts when unit sizes match.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| impl PartialOrd for AssetCapacity { | ||
| fn partial_cmp(&self, other: &Self) -> Option<Ordering> { | ||
| Some(self.cmp(other)) | ||
| } | ||
| } | ||
|
|
||
| impl Ord for AssetCapacity { | ||
| fn cmp(&self, other: &Self) -> Ordering { | ||
| match (self, other) { | ||
| (AssetCapacity::Continuous(a), AssetCapacity::Continuous(b)) => a.total_cmp(b), | ||
| (AssetCapacity::Continuous(a), AssetCapacity::Continuous(b)) => a.partial_cmp(b), | ||
| (AssetCapacity::Discrete(units1, size1), AssetCapacity::Discrete(units2, size2)) => { | ||
| Self::check_same_unit_size(*size1, *size2); | ||
| units1.cmp(units2) | ||
| // NB: Also returns `None` if either is NaN | ||
| (*size1 == *size2).then(|| units1.cmp(units2)) | ||
| } | ||
| _ => panic!("Cannot compare different types of AssetCapacity ({self:?} and {other:?})"), | ||
| _ => None, |
| } | ||
| } | ||
| } | ||
|
|
Description
Copilot had a go at this (#1206), but I didn't like the result.
The reason that
EqandOrdare inconsistent forAssetCapacityis because we are comparing continuous capacities withtotal_cmp, which handles all the weird edge cases of floating-point arithmetic, like NaNs and negative zeroes, giving them a consistent sort order. The defaultPartialEqimplementation that we are deriving has the usual behaviour you get when comparing floating-point numbers (e.g. that comparing NaN with itself returns false), which is not the same.The easy way around this is to just swap
total_cmpforpartial_cmp. I opted to just panic if either of the values is NaN, which is a case that shouldn't arise anyway.Fixes #1162.
Type of change
Key checklist
$ cargo test$ cargo docpresent in the previous release
Further checks