Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix vector sorting with TCO comparators #3256

Merged
merged 4 commits into from
Feb 9, 2022
Merged

Conversation

kustosz
Copy link
Contributor

@kustosz kustosz commented Feb 7, 2022

Pull Request Description

It fixes a bug, see the new test case. Also fixes signatures in one of the dispatch nodes, because they were pointlessly general.

Important Notes

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@kustosz kustosz requested a review from 4e6 as a code owner February 7, 2022 16:50
@radeusgd
Copy link
Member

radeusgd commented Feb 7, 2022

Can we somehow go through other places where we call Enso code in the interpreter to verify that a similar bug is not hiding somewhere there?

@kustosz
Copy link
Contributor Author

kustosz commented Feb 9, 2022

Can we somehow go through other places where we call Enso code in the interpreter to verify that a similar bug is not hiding somewhere there?

I have grepped getCallTarget and there are no other places using this exact form. It is however still possible for other nodes to be broken like this. The only way to do it would be to go through every builtin method to see if it uses tail calls correctly. And there's a looooot of them.

@kustosz kustosz merged commit ee8df25 into develop Feb 9, 2022
@kustosz kustosz deleted the libs/wip/mk/vector_sort_tco branch February 9, 2022 21:17
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants