-
Notifications
You must be signed in to change notification settings - Fork 424
Add a name generator for __get_scope_binding_ functions #1978
Conversation
|
||
this.referentializationState = new Map(); | ||
this._referentializedNameGenerator = referentializedNameGenerator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to be referenced anywhere or do anything. Just wanted to flag this out and make sure it really can be removed safely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be a forgotten residue of earlier code that has been replaced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but please add a test case that fails without this change and passes with it.
|
||
this.referentializationState = new Map(); | ||
this._referentializedNameGenerator = referentializedNameGenerator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be a forgotten residue of earlier code that has been replaced.
this.realm = realm; | ||
} | ||
|
||
_options: SerializerOptions; | ||
scopeNameGenerator: NameGenerator; | ||
scopeBindingNameGenerator: NameGenerator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefix "private" fields with an underscore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of a cosmetic change, so I don't think we need to insist on a regression test case. Let's go ahead with it. Thanks a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@NTillmann is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kwkit is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Release Note: none
This uses a separate name generator for scope access functions.
This addresses #1084 and here is a sample output: