-
-
Notifications
You must be signed in to change notification settings - Fork 312
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
Add Sort ability #6615
base: main
Are you sure you want to change the base?
Add Sort ability #6615
Conversation
Thanks for helping out @jwoudenberg! What's there looks good to me :) |
Thank you @Anton-4! I've started doing the llvm code generation in the last commit. I'd super appreciate a sanity check of my first dive into llvm if you or someone else has some time, just to make sure what I'm doing makes sense. |
Nice work @jwoudenberg! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall yes this makes sense!
// The following calculation will return 0 for equals, 1 for greater than, | ||
// and 2 for less than. | ||
// (a > b) + 2 * (a < b); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason is that this will map directly to the variants that roc would generate for the ordering type, right?
(picking -1, 0, 1 would be more standard in general, but for us would make less sense because of the above)
maybe these functions should return IntValue
to make the idea clearer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason is that this will map directly to the variants that roc would generate for the ordering type, right?
Yes! Thanks for the tip, I'll make them return IntValue
.
Thanks Folkert ❤️ |
Thank you both! |
98a33f9
to
ecef89e
Compare
These tests check the equality functionality. In preparation of adding a Sort ability (which defines a compare method), naming the quality tests `gen_compare` creates ambiguity, so this renames them.
This adds a Sort ability, currently lacking any implementations of the ability.
I'm modeling implementation of the Sort ability for arbitrary types after how it's done for Eq/NotEq.
This implements the following calculation posted by Richard on Zulip: https://roc.zulipchat.com/#narrow/stream/304641-ideas/topic/ordering.2Fsorting.20ability/near/403858126 I'm using the compare.rs module that implements equality as a guide, some the code has some similarities with that, but also I'm starting with a blank page to not overwhelm myself, which might result in some differences (at least at first). The compare.rs module we might want to rename to eq.rs in a future commit. That would free up the name compare.rs to implement generic_eq.
Not sure why, but if I don't choose 108 as the ident id I get an error that structureCompare is missing an implementation.
We had a single test before, but it was failing due to a typo. This fixes the test, plus adds some more for the new sortable types we've added since.
Solves #6551
This is an early work in progress, just to share what's happening so far. Not ready for review (though feedback is always welcome).