Back-end: emit let instead of var.
#6102
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
You can then remove the previous fix (using IIFE) used to capture mutable variable |
cristianoc
added a commit
that referenced
this pull request
Nov 9, 2023
The scope of `var` is per-function, requiring a transformation for closures inside loops when capturing loop variables. This PR turns off the transformation. This PR should go after #6102
|
I wonder how much it'll affect performance. But probably it won't be a big difference. |
|
@DZakh did you see this: #856 (comment) ? |
|
Wow, cool |
cristianoc
added a commit
that referenced
this pull request
Nov 9, 2023
The scope of `var` is per-function, requiring a transformation for closures inside loops when capturing loop variables. This PR turns off the transformation. This PR should go after #6102
|
@cristianoc Could you rebase this? Now that 11.1 is out and development focus is shifting to v12, it would be great to get this merged. |
cristianoc
force-pushed
the
let
branch
2 times, most recently
from
d9a369b
to
698fbf5
Compare
cknitt
reviewed
Apr 28, 2024
|
Sorry, I'm afraid you'll also need to rebase again. |
cristianoc
changed the title
let instead of var.
cristianoc
changed the title
let instead of var.let instead of var.
|
@cknitt ready to go |
cristianoc
added a commit
that referenced
this pull request
Apr 30, 2024
The scope of `var` is per-function, requiring a transformation for closures inside loops when capturing loop variables. This PR turns off the transformation. This PR should go after #6102
cometkim
pushed a commit
to cometkim/rescript
that referenced
this pull request
May 23, 2024
|
Just want to make a quick note that this is improvement is a requirement for the React Compiler it seems to analyze the rules of React. Changing |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #856
Need to check that no cases are missed where this could introduce regressions even if the tests appear to be passing
This will generate a huge amount of noise in the diff w.r.t. other versions of the compiler. It might not be ideal to have this change in an initial version of compiler v11 if other changes are important as they would get lost in the noise
Note that the transformation creating problems in Fix issue with generating async functions inside loops. #6479 can be removed