GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Add support for 'new.target' meta-property by rbuckton · Pull Request #12783 · microsoft/TypeScript Conversation
|
| * @param value The value to append to the array. If `value` is `undefined`, nothing is | ||
| * appended. | ||
| * @param copyOnWrite Indicates whether to return a fresh array rather than modify the | ||
| * existing array. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Looks like this can be removed.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| return false; | ||
| } | ||
|
|
||
| export function getAllLabeledStatements(node: LabeledStatement): { statement: Statement; labeledStatements: LabeledStatement[]; } { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Where is this used?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| // Subtree facts | ||
| NewTarget = 1 << 14, // Contains a 'new.target' meta-property | ||
| NewTargetInComputedPropertyName = 1 << 15, // Contains a 'new.target' meta-property in a computed property name. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Why is it important that this be distinct from NewTarget?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
When we compute flags for a member, such as a method or accessor, it both has its own lexical scope (for the parameters and body) as well as shares the lexical scope of its container (due to the computed property name). Without this flag, encountering a NewTarget in a computed property name would incorrectly indicate that NewTarget was set in the body of the method, not in the container.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I'll have to dive in, but can you explain the es2015 transform changes you needed to make? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
Regarding the changes to the transformer, I am trying to clean up the es2015 transformer and remove some of the variables we use to track state as we descend the tree, this should reduce the overall memory footprint due to values saved on the stack. Ideally I can use this same mechanism to eventually remove some I can isolate these changes to a separate PR if it would be more clear. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| break; | ||
| } | ||
|
|
||
| const captureNewTargetStatement = createVariableStatement( |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
newTarget could be undefined if you're somehow not handling a case - consider adding a
Debug.assert(!!newTarget, "'newTarget' should be defined for " + (ts.SyntaxKind as any)[node.kind])
or something similar.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| return node; | ||
| if (convertedLoopState) { | ||
| if (hierarchyFacts & HierarchyFacts.ArrowFunction) { | ||
| // if the enclosing function is an ArrowFunction is then we use the captured 'this' keyword. |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
"is then" -> ", then"
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| return false; | ||
| } | ||
|
|
||
| export function unwrapInnermostStatmentOfLabel(node: LabeledStatement, beforeUnwrapLabelCallback?: (node: LabeledStatement) => void) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
You only use this function once - do we anticipate using it again?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yes, I also am using this in the asyncGenerators branch
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| || kind === SyntaxKind.TrueKeyword | ||
| || kind === SyntaxKind.SuperKeyword | ||
| || kind === SyntaxKind.NonNullExpression | ||
| || kind === SyntaxKind.MetaProperty |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Do we want to just make this a switch? I think it's asking for it at this point.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I get the crux of the changes which look alright to me - I'm not an expert at the loop conversion code, so Vladimir Matveev (@vladima) may be a good person to check in with. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.