Skip to content

Remove unnecessary unsafe casts in IArray and fix erroneous cast #8196

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

Merged
merged 1 commit into from
Feb 10, 2020

Conversation

LPTK
Copy link
Contributor

@LPTK LPTK commented Feb 4, 2020

There were 61 unnecessary usages of asInstanceOf in IArray.scala. Many of them were completely unneeded, and the rest could be avoided via simple typing tricks, without adding any overhead.

I think unsafe casts should be avoided as much as possible. This is not just a theoretical issue: while making the changes, I discovered a bug in the implementation of IArray#exists (see #8195 (comment) for more info).

I used a private inline implicit conversion for convenience in the IArray object. I tried to use an instance of Conversion instead, but that did not work because of #8194.

Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

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

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

@nicolasstucki
Copy link
Contributor

You will need to adapt TupleOptimizations.scala to pass dotty.tools.backend.jvm.ArrayApplyOptTest

@LPTK
Copy link
Contributor Author

LPTK commented Feb 4, 2020

@nicolasstucki thanks, I've made the changes (boy, that was painful to do).

@LPTK
Copy link
Contributor Author

LPTK commented Feb 6, 2020

@nicolasstucki I've resolved the issues.

@odersky
Copy link
Contributor

odersky commented Feb 10, 2020

Just needs a rebase.

@odersky odersky assigned LPTK and unassigned odersky Feb 10, 2020
@LPTK
Copy link
Contributor Author

LPTK commented Feb 10, 2020

Done.

@smarter smarter merged commit 8f0b68e into scala:master Feb 10, 2020
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.

5 participants