Skip to content

Reduce use of temp variables#1291

Merged
Perryvw merged 3 commits intoTypeScriptToLua:masterfrom
GlassBricks:reduce-temps
Jun 24, 2022
Merged

Reduce use of temp variables#1291
Perryvw merged 3 commits intoTypeScriptToLua:masterfrom
GlassBricks:reduce-temps

Conversation

@GlassBricks
Copy link
Copy Markdown
Contributor

This reduces the use of temp variables by using moveToPrecedingTemp instead of always using a temp. This was done for some cases of assigment and in destructuring declarations.

Additionally, the this keyword (which is read-only) is no longer moved to a temp variable.

@GlassBricks GlassBricks changed the title Reduce use of temp variables in destructured Reduce use of temp variables Jun 22, 2022
@Perryvw
Copy link
Copy Markdown
Member

Perryvw commented Jun 23, 2022

Could you give an example of a case that is not translated differently?

@GlassBricks
Copy link
Copy Markdown
Contributor Author

GlassBricks commented Jun 23, 2022

https://typescripttolua.github.io/play/#code/GYVwdgxgLglg9mABMOcBMAKAhgGkQIwEoBvAKFIF9zRJYFlUMoALGAZwC5EswBPQxGUTDEAGwCmUQbgIVEAXkQt2AbnIjEEBGyn4sAJwVLWbNRozEZ+OYr37Ca0hpTomJvFgDUnwpXJA

Here are some cases where unnecessary temps are used.

Note: with the latest commit, some more temps may be used, but I guess that covers the case where evaluating a.b changes the value of a. If this is not wanted then maybe we should check if temps are needed using isExpressionWithEvaluationEffect instead.

@lolleko
Copy link
Copy Markdown
Member

lolleko commented Jun 23, 2022

I know the validity of these changes is probably already ensured by the existing tests. But I would feel better if we had some explicit functional tests, for your optimization cases, if possible.

@GlassBricks
Copy link
Copy Markdown
Contributor Author

Sure, I just added some tests.

@Perryvw Perryvw merged commit 66bb36e into TypeScriptToLua:master Jun 24, 2022
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