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

Fix #4593: Track askers of hasElidableModuleAccessor #4594

Merged
merged 4 commits into from Nov 30, 2021

Conversation

gzm0
Copy link
Contributor

@gzm0 gzm0 commented Nov 14, 2021

No description provided.

@gzm0 gzm0 requested a review from sjrd November 14, 2021 18:45

var hasElidableModuleAccessor: Boolean = false

var fields: List[AnyFieldDef] = Nil
var fields: List[AnyFieldDef] = linkedClass.fields
Copy link
Member

Choose a reason for hiding this comment

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

The previous setupAfterCreation did not set fields. Is it intentional that, in this refactoring, we now set fields anyway? If yes, why? If not, should it stay Nil (at least in this commit)? Should it be in the next commit instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was concerned about initialization order: In this commit MethodContainer#updateWith will try to set fields during construction.

Probably I should simply invert this commit and the "update field in walkForChanges". That will be cleaner.

Copy link
Member

Choose a reason for hiding this comment

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

That, or initialize it with = _, which will make sure that there is no initialization code in this constructor that will override what updateWith already set before.

This allows us to get rid of one of the matches on this in
`updateWith`.
This does not force the caller to call an "init" method just after
calling the constructor.
@gzm0 gzm0 requested a review from sjrd November 28, 2021 11:13
@sjrd sjrd merged commit 1b1d7e2 into scala-js:master Nov 30, 2021
@gzm0 gzm0 deleted the elidable-module-accessor branch December 4, 2021 16:35
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.

None yet

2 participants