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

Add Sort ability #6615

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Add Sort ability #6615

wants to merge 12 commits into from

Conversation

jwoudenberg
Copy link
Contributor

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).

@Anton-4
Copy link
Collaborator

Anton-4 commented Apr 19, 2024

Thanks for helping out @jwoudenberg! What's there looks good to me :)

@jwoudenberg
Copy link
Contributor Author

jwoudenberg commented May 4, 2024

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.

@Anton-4
Copy link
Collaborator

Anton-4 commented May 4, 2024

Nice work @jwoudenberg!
@folkertdev does the general approach in crates/compiler/gen_llvm/src/llvm/sort.rs look good to you?
All other files seem good :)

Copy link
Contributor

@folkertdev folkertdev left a 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!

Comment on lines +45 to +48
// The following calculation will return 0 for equals, 1 for greater than,
// and 2 for less than.
// (a > b) + 2 * (a < b);
Copy link
Contributor

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

Copy link
Contributor Author

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.

@Anton-4
Copy link
Collaborator

Anton-4 commented May 4, 2024

Thanks Folkert ❤️

@jwoudenberg
Copy link
Contributor Author

Thank you both!

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.
benplotke and others added 6 commits July 14, 2024 05:07
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.
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.

4 participants