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

feat: add TotalOrd library to core::ops for uint types #6635

Merged
merged 16 commits into from
Oct 23, 2024

Conversation

EdwardJES
Copy link
Contributor

@EdwardJES EdwardJES commented Oct 15, 2024

Description

This PR addresses #6597.

A cmp trait has been added that exposes:

  • min(self, Self) -> Self
  • max(self, Self) -> Self

Checklist

  • I have linked to any relevant issues.
  • 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).
  • 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* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

@CLAassistant
Copy link

CLAassistant commented Oct 15, 2024

CLA assistant check
All committers have signed the CLA.

@EdwardJES EdwardJES marked this pull request as ready for review October 15, 2024 04:22
@EdwardJES EdwardJES requested review from a team as code owners October 15, 2024 04:22
@K1-R1
Copy link
Member

K1-R1 commented Oct 17, 2024

Thanks for the PR! We'll be sure to review this soon, but have been mainnet focused this week

Copy link
Contributor

@IGI-111 IGI-111 left a comment

Choose a reason for hiding this comment

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

This is nice, but I'd much rather we get something like Ord+PartialOrd, which is only slightly more code to spin up.

@EdwardJES
Copy link
Contributor Author

EdwardJES commented Oct 19, 2024

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 🙏

@IGI-111
Copy link
Contributor

IGI-111 commented Oct 19, 2024

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.

@EdwardJES EdwardJES changed the title feat: add cmp library to std for uint types feat: add TotalOrd library to std for uint types Oct 19, 2024
@EdwardJES
Copy link
Contributor Author

EdwardJES commented Oct 19, 2024

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.

@EdwardJES EdwardJES requested a review from IGI-111 October 19, 2024 21:32
sway-lib-std/src/lib.sw Outdated Show resolved Hide resolved
@EdwardJES EdwardJES changed the title feat: add TotalOrd library to std for uint types feat: add TotalOrd library to core::ops for uint types Oct 20, 2024
@EdwardJES
Copy link
Contributor Author

Cool, makes sense @IGI-111. Have migrated the trait to core::ops now. Thank you.

@EdwardJES EdwardJES requested a review from IGI-111 October 20, 2024 03:01
@EdwardJES
Copy link
Contributor Author

Will get CI passing in the next day or so.

@EdwardJES EdwardJES requested a review from a team as a code owner October 21, 2024 04:48
@EdwardJES
Copy link
Contributor Author

EdwardJES commented Oct 21, 2024

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)

Copy link
Member

@bitzoic bitzoic left a comment

Choose a reason for hiding this comment

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

This is great! Thank you for the good use of the SRC-2 standard 😁

@IGI-111 IGI-111 merged commit 40e2b30 into FuelLabs:master Oct 23, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants