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

Annex B.3.3.1's monkeypatching of FunctionDeclarationInstantiation is out of date #677

Merged
merged 1 commit into from
Aug 23, 2016

Conversation

jswalden
Copy link
Contributor

It refers to "BoundNames of argumentsList", but argumentsList is now an actual value, not a grammar production. I suspect the name used to be correct, then someone renamed argumentsList to formals, and things got skew. Now parameterNames exists as the "BoundNames of formals", so it's the best thing to use.

I also went and added big WARNING comments next to all the monkey-patched spec algorithms, so maybe renames of stuff will remember to update Annex B as well. There were already NOTE: steps that theoretically should have been enough reminder to the spec-modifier, but I guess they weren't. It seems to me like a comment at the start of the algorithms is most likely to be read by someone skimming the algorithms to see where to do variable renames, so it's worth adding even atop the NOTEs.

…le in FunctionDeclarationInstantiation's body, and add warning comments by all the spec algorithms monkey-patched by Annex B so that future refactoring doesn't create broken/dangling references in Annex B.
@jmdyck
Copy link
Collaborator

jmdyck commented Aug 23, 2016

In ES6 rev33, it said "... BoundNames of FormalParameters of g".
In rev34, it changed to "... BoundNames of argumentsList".

The change appears to relate to https://bugs.ecmascript.org/show_bug.cgi?id=3490#c6

Since we know we are dealing with a FunctinoDeclaration we know that argumentsList is a FormalParameters

@littledan
Copy link
Member

LGTM. This patch illustrates one of the hazards of having normative optional text out of line. I wonder if we could have it inline along the lines of what we agreed for 402 (but have not yet implemented) at tc39/ecmarkup#96 .

@bterlson bterlson merged commit d7534cc into tc39:master Aug 23, 2016
@bterlson
Copy link
Member

I'm not sure inline normative-optional would help much - you still have to look around your location and read everything to ensure you haven't broken anything. Comments are at least as good of a solution IMO. Perhaps there is a better way to solve this with some additional markup and/or tooling... will ponder.

@jswalden jswalden deleted the bad-annexb-reference branch August 23, 2016 20:09
jaychsu added a commit to jaychsu/ecma262 that referenced this pull request Sep 7, 2016
* upstream/master: (301 commits)
  Editorial: remove mistakenly committed change to eval-related super errors
  Normative: Point to the latest version of UTR15 (tc39#681)
  Meta: update ecmarkup version dependency (tc39#682)
  Editorial: Update a variable name in Annex B.3.3.1 to refer to the proper variable in FunctionDeclarationInstantiation's body, and add warning comments by all the spec algorithms monkey-patched by Annex B so that future refactoring doesn't create broken/dangling references in Annex B. (tc39#677)
  Meta: update ecmarkup version dependency
  Editorial: dfn-ify 'goal symbol'
  Editorial: TypeArray -> TypedArray (tc39#675)
  Editorial: Correct a wrong cross-reference in Annex C (tc39#674)
  Editorial: tweak whitespace and articles (tc39#667)
  Editorial: Tweak EnumerateObjectProperties informative definition (tc39#656)
  Editorial: Rename variable (indx --> index) (tc39#658)
  Editorial: Fix reference in ScriptEvaluation (tc39#659)
  Editorial: Consistent `undefined or null` order (tc39#660)
  Editorial: Refactor Array.prototype.toLocaleString (tc39#662)
  Editorial: Tweak Array.prototype.join (tc39#661)
  Simplify description of CreateImmutableBinding (tc39#654)
  Normative: Resolve template argument references (tc39#609)
  Editorial: Refactor Array.prototype.join (tc39#638)
  Editorial: contains --> Contains in ModuleListItem SS
  Normative: Allow initializers in ForInStatement heads (tc39#614)
  ...
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.

4 participants