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

Scala.js trait super call code generation #7962

Merged
merged 1 commit into from
Jan 17, 2020
Merged

Scala.js trait super call code generation #7962

merged 1 commit into from
Jan 17, 2020

Conversation

molikto
Copy link
Contributor

@molikto molikto commented Jan 11, 2020

In this commit scala.js will not rewrite super calls into scala2x traits to static methods. so <init> trait constructors is not rewritten, but although they are non-static in scala2x, they are actually named $init$.

Alternative fix will be changing encodeMethodSym (I only thought of this alternative afterwards, not sure it works actually), not sure which one is preferred...

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! ☀️

Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, and sorry for the delay. Here are a few comments. The overall idea looks sound.

compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala Outdated Show resolved Hide resolved
@molikto
Copy link
Contributor Author

molikto commented Jan 15, 2020

@sjrd Hello! After getting more understanding of the compiler code, I got an alternative fix, which is smaller code change, although it adds one more bit of code path difference between JVM and js

@molikto molikto requested a review from sjrd January 15, 2020 04:50
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Thanks. The new fix looks good.

Could you address the two whitespace-related comments below, then squash everything together in one commit, please? Given that the strategy has been completely replaced, it makes no sense to keep in the intermediate commits.

project/Build.scala Outdated Show resolved Hide resolved
compiler/src/dotty/tools/backend/sjs/JSCodeGen.scala Outdated Show resolved Hide resolved
@molikto molikto requested a review from sjrd January 17, 2020 11:20
Copy link
Member

@sjrd sjrd left a comment

Choose a reason for hiding this comment

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

Thanks!

@sjrd sjrd merged commit 1cff48f into scala:master Jan 17, 2020
@molikto molikto deleted the try-fix-trait-init branch January 18, 2020 03:54
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.

3 participants