-
Notifications
You must be signed in to change notification settings - Fork 553
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
Added List.sort(comp) to List module #802
Conversation
Added `sort(comp)` function to List using an in-place quick sort implementation.
Your doc example shows it, but it would be nice to have some documentation on the expected behavior of |
added default comp funcion `List.sort()`. added more documentation describing `comp`.
Thanks for pointing that out. I just updated the docs and added a default |
From my experience at using the language, I would recommend to change So as a rule of thumb; if you write a one liner, it is highly likely you want to return the computed value, so use the real one liner expression (that implicitly returns the computed value). I know, it is not really intuitive or does not scale producing changes, but the language is like this... |
Just to clarify you mean a uni-line (stub function) like so |
Yes this one, it act as a stub with implicit returning.
|
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.
Some comments based on code review, I didn't test it.
projects/xcode/wren_test.xcodeproj/project.xcworkspace/contents.xcworkspacedata
Outdated
Show resolved
Hide resolved
Does the tests pass now ? |
Not with the stack corruption issue |
I think the test should be a bit more atomic to be more interesting. |
Added `sort(comp)` function to List using an in-place quick sort implementation.
added default comp funcion `List.sort()`. added more documentation describing `comp`.
I rebased from |
@mathewmariani It seems you did a rebase followed by a merge. Please get rid of the merge for force-push just the rebased version of your branch. |
bbc04d6
to
d1d6287
Compare
I believe that's it |
Should I resolve all the comments, or leave that up to the commenters? |
@mathewmariani I see most of my comments marked as "Outdated" and I see no way to resolve them (neither the ones that haven't been marked outdated), so I guess that's on you. |
Anything else needed for this to be merged? |
Nice work. Three things, however:
|
I don't think it's semantically appropriate to have a Sequence.sort method, because a sequence could in theory be infinite in length (and they can be lazily evaluated), so a sorting operation would be impossible or intractable. |
Also a sort function in c runs into reentrancy (specifically, it's not possible when a user supplied compare function is involved) |
@ruby0x1 I think the reentrancy is about to be solved, am I wrong? @avivbeeri I know that, be that still holds in Python for example, and it does have a |
@ChayimFriedman2 I implemented Quick Sort since it's the algorithm used in Lua's |
I'll look closely soon, busy with some other Wren stuff locally. |
To help comprehension, could you replace 'a' and 'b' with something like
'i' and 'j' where iterators are expected.
|
I think |
@ChayimFriedman2 @mhermier I'm leaning towards |
@mathewmariani code is for humans, not machines, legibility > compactness. Especially for wren, which strives to help people learn and understand, making the code clearer is part of that goal. In my code I used |
changed variables names from `a`, `b` to `low`, `high` to increase readability
Updated the documentation for `list.sort()` to better explain the boolean result and the nature of the compare function
Thanks @mathewmariani , I'll probably tweak some things after merging, but looks good from here. |
Added
sort(comp)
function to List using an in-place quicksort implementation.