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.

Sign up for GitHub

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

Jump to bottom

Editorial: quick fixes for recent merges #1787

Merged
merged 4 commits into from
Feb 19, 2020
Merged

Editorial: quick fixes for recent merges #1787

merged 4 commits into from
Feb 19, 2020

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Nov 19, 2019 •
edited
Loading

The phrase "the Number value for X" is only defined on mathematical values,
but _prim_ is a BigInt value here.
So we have to take the mathematical value of _prim_
before applying "the Number value for ..."

(Quick fix for PR #1766.)

@michaelficarra
Copy link
Member

Could we instead define "the Number value" for a BigInt in terms of "the mathematical value of"?

@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 20, 2019

We could, but I don't think it would be a good idea.

@ljharb
Copy link
Member

ljharb commented Nov 20, 2019

Why not?

@jmdyck
Copy link
Collaborator Author

jmdyck commented Nov 21, 2019

  • It would complicate the 'signature' and definition of "the Number value for X".
  • It would make the story for converting between different numeric types less coherent, less consistent.
  • It would set an unnecessary precedent for future numeric types.

And for what benefit? Just to make one algorithm shorter by 4 words?

@jmdyck jmdyck changed the title Editorial: Insert the mathematical value of Editorial: quick fixes for recent merges Dec 12, 2019
@jmdyck
Copy link
Collaborator Author

jmdyck commented Dec 12, 2019

Added 6 commits relating to yesterday's merges.

@ljharb
Copy link
Member

ljharb commented Dec 12, 2019

I'm fine with all the commits except that I'm unsure about "eliminate grammatical parameters" (d7745ff). Can you explain that one a bit?

@jmdyck
Copy link
Collaborator Author

jmdyck commented Dec 13, 2019

Given a production that is defined with grammatical parameters, when we recap it (e.g., to associate it with an early error rule or an SDO algorithm), we almost always omit the grammatical parameters and other 'annotations'.

As 5.2.2 Syntax-Directed Operations says:

When an algorithm is associated with a production alternative, the alternative is typically shown without any “[ ]” grammar annotations. Such annotations should only affect the syntactic recognition of the alternative and have no effect on the associated semantics for the alternative.

For example, see the Early Errors for UpdateExpression, where you can compare the productions to the defining productions immediately prior.

@ljharb
Copy link
Member

ljharb commented Dec 13, 2019

So, is that why it's editorial - because all of those tags are correct but implied and thus redundant?

@jmdyck
Copy link
Collaborator Author

jmdyck commented Dec 13, 2019

Yup.

@ljharb ljharb requested review from michaelficarra, syg, bakkot and a team December 14, 2019 00:32
@ljharb ljharb self-assigned this Dec 14, 2019
@jmdyck
Copy link
Collaborator Author

jmdyck commented Dec 28, 2019

Added two commits re Punctuator in the lexical grammar: one to eliminate an ambiguity introduced in PR #1646, and one to do some refactoring while I was in the neighborhood.

@ljharb
Copy link
Member

ljharb commented Dec 28, 2019

I'm not certain about your number 2 in 74a1046 - it seems clearer to me to have OptionalChainingPunctuator as a separate thing, even if it's only used in one place.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Dec 28, 2019

I disagree, but not enough to fight it, so I've reinstated OptionalChainingPunctuator.

ljharb pushed a commit that referenced this pull request Jan 2, 2020
@jmdyck @ljharb
Markup: insert 3 missing end-tags (#1787)
f4d40a5 
(from PR #1062 and PR #1646)
ljharb pushed a commit that referenced this pull request Jan 2, 2020
@jmdyck @ljharb
Markup: change id attribute of 2 new emu-clauses (#1787)
384978f 
... to match the clause title (operation name).
(from PR #1646)
@ljharb
Copy link
Member

ljharb commented Jan 2, 2020

I've pulled the two markup fix commits directly into master, and rebased this PR.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 22, 2020

Added a commit re whitespace in <emu-grammar> content.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Jan 23, 2020

Added a commit to fix a couple things from PR #1406.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Feb 13, 2020

Added a tiny commit to remove an extraneous comma.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Feb 15, 2020

(force-pushed to resolve a merge conflict)

syg
syg approved these changes Feb 19, 2020
View reviewed changes
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, thanks for the many small fixes

@jmdyck @ljharb
Editorial: Insert the mathematical value of (tc39#1787)
31bc4fb 
The phrase "the Number value for X" is only defined on mathematical values,
but `_prim_` is a BigInt value here.
So we have to take the mathematical value of `_prim_`
before applying "the Number value for ..."
@jmdyck @ljharb
Editorial: fixes from tc39#1646, tc39#1644, tc39#1406 (tc39#1787)
97d8f19 
 - insert space before paren in clause heading
 - eliminate grammatical parameters from non-defining production
 - Add 4 <emu-prodref> elements to Annex A
 - tweak syntax in algorithms
 - eliminate accidental ambiguity in Punctuator
      PR tc39#1646 introduced an ambiguity to the lexical grammar:
      Punctuator derives OtherPunctuator in two different ways --
      directly, and indirectly via OptionalChainingPunctuator.

      It doesn't make sense for OptionalChainingPunctuator to derive OtherPunctuator,
      so I've eliminated that alternative.
 - minor grammar refactoring re Punctuator
      Move the Punctuator production back to its former position.
      (Grammars are generally written top-down.)
 - Fix a 'use' production for AsyncConciseBody to match the new defining production
 - Add an <emu-prodref> for ExpressionBody to Annex A
@jmdyck @ljharb
Editorial: Consistify whitespace in <emu-grammar> content (tc39#1787)
107797c 
About half of the changes delete an extraneous space
at the end of a lookahead annotation. The rest are misc.
None of them will make a visible difference in the rendered version.
@jmdyck @ljharb
Editorial: remove extraneous comma (tc39#1787)
0c9930d
@ljharb ljharb merged commit 0c9930d into tc39:master Feb 19, 2020
@jmdyck jmdyck deleted the editorial branch February 20, 2020 20:15
@jmdyck jmdyck mentioned this pull request Jun 30, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vicheus Vicheus Vicheus approved these changes

@bakkot bakkot bakkot approved these changes

@michaelficarra michaelficarra michaelficarra approved these changes

@syg syg syg approved these changes

@ljharb ljharb ljharb approved these changes

Assignees

@ljharb ljharb

Labels
editorial change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jmdyck @michaelficarra @ljharb @syg @bakkot @Vicheus