Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Editorial: Correct and make readable for-in/of/await-of static semantics #1284

Merged
merged 2 commits into from
Oct 10, 2019

Conversation

TimothyGu
Copy link
Member

@TimothyGu TimothyGu commented Aug 10, 2018

First commit:

Currently, VarScopedDeclarations is defined for

IterationStatement :
  `for` `await` `(` `var` ForBinding `of` AssignmentExpression `)` Statement

in two different places with conflicting definitions. Remove the incorrect definition.

Second commit:

Coalesce children of for-in, for-of, and for-await-of statements with identical definitions for VarDeclaredNames and VarScopedDeclarations for better readability: instead of having six different algorithm blocks for each of VarDeclaredNames and VarScopedDeclarations now there are only two for each.

Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

several children have duplicated definition of VarScopedDeclarations

Not "several", but one RHS (for await ( var ForBinding of ...) has two conflicting definitions. This is clearly an editorial mistake, but I'm not certain if the fix qualifies as editorial, since an implementation that happened to follow the wrong definition would now have to change their code. (But hopefully test262 has tests that only admit the correct choice.)

The first commit eliminates the first of the conflicting definitions, which looks like the correct fix to me. However, the commit somewhat obscures that fix by also moving a different RHS from one group to another. It would be clearer to accomplish that move in the second commit. Also, the commit message should maybe be more specific about what it's fixing.

The second commit merges groups that have the same algorithm, which is purely editorial, and should indeed improve readability. However, in a multi-RHS production, I think we normally put the RHSs in the same order as they appear in the defining production.

while some have none.

No, I don't think any RHSs are missing. ( If you're thinking of the for(;;) RHSs, they're handled separately in 13.7.4.)

@TimothyGu TimothyGu force-pushed the iteration-editorial branch from 86aab89 to 007386c Compare August 10, 2018 18:17
@TimothyGu
Copy link
Member Author

Not "several", but one RHS (for await ( var ForBinding of ...) has two conflicting definitions.

Indeed. Commit message fixed.

but I'm not certain if the fix qualifies as editorial, since an implementation that happened to follow the wrong definition would now have to change their code.

I'll let the editors decide on this, but my belief is that it is editorial.

The first commit eliminates the first of the conflicting definitions, which looks like the correct fix to me. However, the commit somewhat obscures that fix by also moving a different RHS from one group to another. It would be clearer to accomplish that move in the second commit.

Done.

No, I don't think any RHSs are missing.

Commit message has been changed.

@ljharb ljharb requested a review from jmdyck October 4, 2019 02:08
Copy link
Collaborator

@jmdyck jmdyck left a comment

Choose a reason for hiding this comment

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

Looks good.

@ljharb ljharb requested a review from syg October 8, 2019 20:45
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

lgtm

…emantic for IterationStatement (tc39#1284)

Currently, VarScopedDeclarations is defined for

  IterationStatement :
    `for` `await` `(` `var` ForBinding `of` AssignmentExpression `)` Statement

in two different places with conflicting definitions. Remove the
incorrect definition.
Coalesce children of `for`-`in`, `for`-`of`, and `for`-`await`-`of`
statements with identical definitions for VarDeclaredNames and
VarScopedDeclarations for better readability: instead of having six
different algorithm blocks for each of VarDeclaredNames and
VarScopedDeclarations now there are only two.
@ljharb ljharb self-assigned this Oct 10, 2019
@syg syg force-pushed the iteration-editorial branch from 007386c to fa8e6b3 Compare October 10, 2019 23:06
@syg syg merged commit fa8e6b3 into tc39:master Oct 10, 2019
@TimothyGu TimothyGu deleted the iteration-editorial branch October 11, 2019 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants