-
Notifications
You must be signed in to change notification settings - Fork 5
feat: add quick_sort and quick_sort_by implementations #133
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
Conversation
WalkthroughIntroduces two new public macros Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (30.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #133 +/- ##
===========================================
+ Coverage 74.82% 95.29% +20.47%
===========================================
Files 13 19 +6
Lines 433 1468 +1035
Branches 110 385 +275
===========================================
+ Hits 324 1399 +1075
+ Misses 106 48 -58
- Partials 3 21 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@math/core/sources/internal/macros.move`:
- Around line 988-1038: The documentation incorrectly claims stability and
left-first processing for the quick_sort<$Int> macro; the implementation using
Lomuto and an explicit LIFO stack is not stable and currently processes the
right partition first. Fix by (a) removing or changing the stability/left-first
wording in the macro doc comments, and (b) change the stack push order in
quick_sort so the larger partition is pushed first and the smaller partition
pushed second (so the smaller partition is popped/processed next) — update the
push_back sequence around the partition index returned by partition!(vec, start,
end) (references: quick_sort macro, partition! invocation, stack_start and
stack_end vectors and their push_back calls).
In `@math/core/tests/macros_tests.move`:
- Around line 1854-1860: Reformat the Move test file to satisfy Prettier: run
the provided formatter (npx prettier-move -w **/*.move) or apply equivalent
formatting to the block around the sorting assertion (the while loop using
variables vec, len, i and the assert! call) so spacing/line breaks match project
style, then re-commit the updated file.
This reverts commit 9bf7065.
ericnordelo
left a comment
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.
Looking good, we also need to make this function accesible by exposing it in the corresponding integer modules.
|
Added |
bidzyyys
left a comment
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.
Great job @qalisander!
Co-authored-by: Eric Nordelo <[email protected]>
ericnordelo
left a comment
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.
LGTM!
Casual quick sort in-place implementation similar to openzeppelin-solidity qsort.
Compare to referenced (in issue) implementation it utilities stack data structure, since call stack (recursion) is not supported within move macro. Move macro cannot call itself.
Resolves #95
PR Checklist
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.