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

assert that ‘sort’ and ‘sortBy’ do not rely on Array#sort being stable #94

Merged
merged 1 commit into from
Apr 1, 2018

Conversation

davidchambers
Copy link
Member

While working on #92 I realized I could replace idx: rs.length with idx: 0 and all the tests would still pass. This is because Array#sort is stable in my environment, as it is in most environments. The language specification does not guarantee this, though, so we should not assume it.

This pull request introduces a function, withUnstableArraySort, which monkey patches Array#sort, evaluates a thunk, then restores Array#sort to its prior value. Any change to Z.sort or Z.sortBy which assumes Array#sort to be stable will pass tests in the default environment but will fail some tests in the modified environment.

/cc @paldepind

return this;
};
thunk();
Array.prototype.sort = Array$prototype$sort;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is not quite right. If an exception is thrown during the evaluation of thunk() we will fail to restore the original value.

@davidchambers davidchambers force-pushed the davidchambers/unstable branch from 27fb222 to d41050e Compare April 1, 2018 10:01
@davidchambers davidchambers merged commit cd7f3de into master Apr 1, 2018
@davidchambers davidchambers deleted the davidchambers/unstable branch April 1, 2018 10:05
@paldepind
Copy link
Contributor

Sorry, I'm late 😭

This is because Array#sort is stable in my environment, as it is in most environments.

Array#sort is unstable in V8 from what I can find.

In any case, I think this PR is a very good idea to make the sort predictably unstable 👍

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.

2 participants