Skip to content

Hoisting#369

Merged
tomblind merged 33 commits intomasterfrom
hoisting
Feb 6, 2019
Merged

Hoisting#369
tomblind merged 33 commits intomasterfrom
hoisting

Conversation

@tomblind
Copy link
Copy Markdown
Collaborator

No description provided.

@tomblind tomblind requested review from Perryvw and lolleko January 28, 2019 23:57
return tstl.createBinaryExpression(expression, tstl.createNumericLiteral(1), tstl.SyntaxKind.AdditionOperator);
}

protected findScope(...scopeTypes: ScopeType[]): Scope | undefined {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be interesting to use a bitmask for this instead of an array.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

that's a good idea!

- checking variables for reference-before-declaration when they are first seen to avoid scanning later
- caching nested functions in scope to avoid repeat scan later
- added error for attempting to hoist when hoisting is off since it's easy to detect now
- added tests for hoisting errors
@tomblind
Copy link
Copy Markdown
Collaborator Author

tomblind commented Feb 1, 2019

Ok, this PR is probably worth reviewing again.

There are now 3 options for hoisting:

  • None : No hoisting is performed at all. Errors are thrown when attempting to reference something before it's declared
  • Full : Everything is hoisted. No additional checks required, but produces ugly/slower lua
  • Required : Only variables/functions referenced before their declaration or from another hoisted function will be hoisted. Requires compiler overhead, but produces best results.

type: "boolean",
},
hoisting: {
alias: "h",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

h is usually help, maybe choice something different or don't set an alias at all.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

good point

src/LuaAST.ts Outdated
}
if (!node.tsOriginal) {
node.tsOriginal = parent.tsOriginal;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is intentionally avoided to link the Lua AST node to the tsOriginal. Is this really required?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think I can rework things to avoid it, but I'm curious what the reason is for avoiding it? Seems like it would be quite useful.

expression.text = text as string;
expression.symbol = symbol;
return expression;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Again is it possible to avoid having ts types in the Lua AST?
Sorry I haven't elaborated on why I don't think TS stuff should be in an Lua AST.

Main reason, it's a design choice: Having no TS nodes in the AST clearly separates the transform and print step. Allowing them to run independent of each other. Not having any TS nodes in the AST allows us to easily create Lua ASTs from the ground up (with no TS used to generate the AST).

I think a good compromise would be do just save the symbolID on the AST node, since that would just be a number and not a ts type. And as far as I can tell you only save the symbol to keep track on what should be hoisted? I think the ID could be enough for that purpose.
Or maybe map the tstl.Identifier to the Symbol somewhere else? For example in a Map<> in the Transformer, that way we wouldn't have to store the information on the node itself. Since you don't need that info in there print step there is no reason to save it on the node tbh.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ok, thanks for the explanation - that makes sense to me. All I need here is a way to differentiate unique symbols, so an ID system like you suggest should work.

@tomblind tomblind merged commit 313e6ea into master Feb 6, 2019
@tomblind tomblind deleted the hoisting branch February 6, 2019 19:28
@tomblind tomblind mentioned this pull request Feb 6, 2019
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