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: quick fixes #1965

Merged
merged 1 commit into from
May 6, 2020
Merged

Editorial: quick fixes #1965

merged 1 commit into from
May 6, 2020

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Apr 23, 2020

... for some recent and not-so-recent commits.

Fixes: #1971

@michaelficarra
Copy link
Member

The grammar parameters commit goes against our editorial resolution to have grammar parameters always appear in SDO definitions (in the production name and in each alternative, but not any alternative "guards"). Please drop that commit.

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

LGTM other than the grammar flags commit.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 24, 2020

The grammar parameters commit goes against our editorial resolution to have grammar parameters always appear in SDO definitions

Hm. Was that resolution stated somewhere? Note that 5.2.2 Syntax-Directed Operations still says:

When an algorithm is associated with a production alternative, the alternative is typically shown without any “[ ]” grammar annotations.

@michaelficarra
Copy link
Member

@jmdyck It was in our editor's update slides and we discussed it again 15 days ago but I seem to have forgotten to post the resolution in that thread. Anyway, I guess the above comment will suffice.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 24, 2020

@jmdyck It was in our editor's update slides [..] but I seem to have forgotten to post the resolution in that thread. Anyway, I guess the above comment will suffice.

On that slide, you're presumably referring to the bullet that says:

ecma262 #1885: include flags and subscripts in SDO grammar snippets

So all I got from #1885 and that bullet was that productions should include grammatical parameters when the accompanying prose references them. Now you're saying that every production in an SDO or Early Error rule should include its grammatical parameters? I count roughly 2000 productions where the parameters would have to be reinserted. Why do you want that? (And do you think anyone will be willing to do the work?)

@bakkot
Copy link
Contributor

bakkot commented Apr 24, 2020

Yeah, I also saw that it was going to be thousands of places and reconsidered. I'll bring it up again sometime.

And do you think anyone will be willing to do the work?

I could do it automatically with ecmarkup at this point, I think.

@michaelficarra
Copy link
Member

@jmdyck At least one of the mentioned motivations during the editor call was the fact that readers have many times been confused about why the parameters were not present in the SDO definition and asked about this. I can look for examples if you like, but it might be hard to search for them.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 24, 2020

(force-pushed after rebasing to master and adding a commit.)

@ljharb ljharb requested review from syg, ljharb and a team April 27, 2020 07:31
Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

Please drop the commit that removes the grammar parameters in SDOs.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 30, 2020

Please drop the commit that removes the grammar parameters in SDOs.

I disagree with the rationale, but I'll drop it if that's the consensus of the editors. (So far, I'm getting mixed signals.)

@jmdyck
Copy link
Collaborator Author

jmdyck commented Apr 30, 2020

(force-pushed to rebase to master and add a commit to do another step-reference fix)

@bakkot
Copy link
Contributor

bakkot commented May 1, 2020

I'll drop it if that's the consensus of the editors. (So far, I'm getting mixed signals.)

The editor group will talk about it again at some point, but until then let's avoid churning it - that is, I would also prefer you drop the commit in question, for now; we can always add it again later if we decide we are in agreement to omit all parameters.

@jmdyck
Copy link
Collaborator Author

jmdyck commented May 1, 2020

Fair enough.

@jmdyck
Copy link
Collaborator Author

jmdyck commented May 1, 2020

(force-pushed to remove the "Drop grammatical parameters" commit)

Editorial: Reinstate an SDO rule

PR tc39#1933 deleted SDO rules that are handled by the chain production rule,
but it also deleted this one which isn't.
(It has "TV" on the left and "TRV" on the right.)

Editorial: Move an <emu-note> element

PR tc39#1490 (among other things) moved the evaluation semantics for ArrowFunction
from the Evaluation SDO to the NamedEvaluation SDO.
The accompanying <emu-note> should have moved at the same time
(in particular because of the reference to "step 3").

Editorial: Delete <emu-note> in TimeClip clause

PR tc39#1827 (among other things) removed step 4 from the algorithm for TimeClip,
obsoleting the accompanying emu-note that describes "the point of step 4".

Conceivably, the note could be reworded to describe the effect of
'ToInteger' on step 3, but I don't think it'd be worth the bother.

Editorial: Change "Step 2.a" to "Step 2.b" in RepeatMatcher note

PR tc39#1889 (among other things) inserted a step before the former 2.a,
but didn't update the note that referenced it.

Editorial: Change step 7 to step 6 in SortCompare note

Commit 9c1e076 (2015-10-26) introduced the '?' abbreviation for ReturnIfAbrupt.
This caused the ToString call on step 7 to move to step 6,
but the note that referred to it wasn't updated.

Editorial: Fix typo: "Descritor" -> "Descriptor"

Editorial: Fix typo: "GeneratorObject" -> "generator object"

(There's no such thing as a GeneratorObject.)

Editorial: Delete "as a parameter" after "is present"

(It's the only place in the spec where we use that phrasing.)

Editorial: Change "which" to "that"

... in "{String,Array,Map,Set} methods which return such iterators"

Editorial: Insert a comma in SetDefaultGlobalBindings()

Formerly, it read like "containing" modified "the property",
when it actually modified "the property descriptor".

Editorial: Change "lexical environment" to "Environment Record"

... in FunctionDeclarationInstantiation,
to balance the NOTE in the other arm of the if-else,
and also for consistency with the NOTE at 27.a.

(I should have done this in PR tc39#1697.)

Editorial: Change "step 3" to "step 4" in Note

... that accompanies the NamedEvaluation semantics for
    ArrowFunction : ArrowParameters `=&gt;` ConciseBody

PR tc39#1870 (among other things) inserted a step before the former step 3,
but didn't update the note that referenced it.
@ljharb ljharb merged commit c112315 into tc39:master May 6, 2020
@jmdyck jmdyck deleted the editorial branch May 7, 2020 01: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.

Discrepancy in the note for TimeClip
5 participants