sway
sway copied to clipboard
feat: add `TotalOrd` library to `core::ops` for `uint` types
Description
This PR addresses https://github.com/FuelLabs/sway/issues/6597.
A cmp trait has been added that exposes:
min(self, Self) -> Selfmax(self, Self) -> Self
Checklist
- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand areas.
- [ ] I have updated the documentation where relevant (API docs, the reference, and the Sway book).
- [ ] If my change requires substantial documentation changes, I have requested support from the DevRel team
- [x] I have added tests that prove my fix is effective or that my feature works.
- [ ] I have added (or requested a maintainer to add) the necessary
Breaking*orNew Featurelabels where relevant. - [x] I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
- [x] I have requested a review from the relevant team or maintainers.
Thanks for the PR! We'll be sure to review this soon, but have been mainnet focused this week
This is nice, but I'd much rather we get something like
Ord+PartialOrd, which is only slightly more code to spin up.
Thanks for the feedback, @IGI-111. Just before I make this change. I'd just like to run my understanding by you. Extend this current trait (cmp (maybe this name is not appropriate given Ord and PartialOrd have cmp in their methods)) to have functionality from Ord and PartialOrd, and introduce the Ordering enum.
I'm a little confused as to where the PartialOrd.ge, PartialOrd.le would be written as the Ord trait in the core lib implements lt, gt already.
I would appreciate any help on the above, thank you 🙏
Ah yes, it's unfortunate that we already added an Ord that doesn't actually map onto Rust's Ord.
What I would prefer is if we can achieve that same level of functionality and have two different traits for total and partial orders. But I suppose that's what your existing code is already trying to achieve with Cmp mapping to Ord and Ord mapping to PartialOrd.
I think we can just merge Cmp as is or with a name change (TotalOrd?) and implement the Ordering functionality later in a breaking change that actually restores the expected names for people who are coming from Rust.
So really, forget what I said and maybe change the name of the trait to TotalOrd and we're good to go for this.
Thanks for the feedback @IGI-111, I've renamed occurrences of cmp to TotalOrd.
Agree that there is a nice DX gain from maintaining a mapping between Rust. Let me know if you'd like me to capture that future change as an issue.
Cool, makes sense @IGI-111. Have migrated the trait to core::ops now. Thank you.
Will get CI passing in the next day or so.
Thanks @K1-R1, @IGI-111 I've got the tests failing on CI passing locally.
Bit of a battle with formatting when the test.toml validates the ABI. (I think me adding the newlines was causing it to fail)