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: misc fixes #1636

Merged
merged 5 commits into from
Sep 16, 2019
Merged

Editorial: misc fixes #1636

merged 5 commits into from
Sep 16, 2019

Conversation

jmdyck
Copy link
Collaborator

@jmdyck jmdyck commented Jul 18, 2019

.. re PR #1571
... and PR #1376.

@ljharb ljharb requested review from zenparsing and a team July 18, 2019 17:38
@michaelficarra
Copy link
Member

Wow I can't believe we missed these.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Aug 8, 2019

Added 3 commits relating to the merge of PR #1376.

@@ -19962,7 +19962,7 @@ <h1>Runtime Semantics: ClassDefinitionEvaluation</h1>
<pre><code class="javascript">constructor() {}</code></pre>
using the syntactic grammar with the goal symbol |MethodDefinition[~Yield, ~Await]|.
1. Set the running execution context's LexicalEnvironment to _classScope_.
1. Let _constructorInfo_ be DefineMethod of _constructor_ with arguments _proto_ and _constructorParent_.
1. Let _constructorInfo_ be ! DefineMethod of _constructor_ with arguments _proto_ and _constructorParent_.
Copy link
Member

Choose a reason for hiding this comment

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

Using the ?/! feels awkward with this form - meaning where it doesn't look like a JS function invocation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well, ...

Copy link
Member

Choose a reason for hiding this comment

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

True enough :-) it still looks weird to me.

spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Outdated
@@ -24768,7 +24768,7 @@ <h1>Object.prototype.valueOf ( )</h1>
<emu-alg>
1. Return ? ToObject(*this* value).
</emu-alg>
<p>This function is the <dfn>%ObjProto_valueOf%</dfn> intrinsic object.</p>
<p>This function is the <dfn>%Object.prototype.valueOf%</dfn> intrinsic object.</p>
Copy link
Member

Choose a reason for hiding this comment

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

please revert this; as long as %ObjProto_valueOf% and similar exists in the table, these prose comments referencing it should remain.

Copy link
Collaborator Author

@jmdyck jmdyck Aug 23, 2019

Choose a reason for hiding this comment

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

You mean references within <dfn>? I addressed that in the commit message.

Copy link
Member

Choose a reason for hiding this comment

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

I mean prose that mentions them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, but why?

Copy link
Member

Choose a reason for hiding this comment

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

This entire line isn't needed unless %ObjProto_valueOf% exists, because it's already implied by being reachable from %Object%. As such, these lines should remain untouched, until a future PR removes both them and the legacy intrinsics.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah! I had wondered about the tautology of X.Y is the intrinsic object %X.Y%.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But wait, in f1b22ef, you changed a bunch of prose-y references from old-style to new-style. Is there something different about them, or was that just a mistake? And do you want me to revert them back to old-style too?

Copy link
Member

Choose a reason for hiding this comment

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

I changed all of them except the ones that were in these bullet lists, defining the objects at their creation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What about line 24664 (is the intrinsic object <dfn>%Object.prototype%</dfn>)
and line 38776 (is the intrinsic object <dfn>%AsyncFunction.prototype%</dfn>)?

Copy link
Member

Choose a reason for hiding this comment

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

oops, looks like i mistakenly changed those. sorry for the confusion

spec.html Outdated
@@ -25819,7 +25819,7 @@ <h1>Number.prototype</h1>
<h1>Properties of the Number Prototype Object</h1>
<p>The Number prototype object:</p>
<ul>
<li>is the intrinsic object <dfn>%NumberPrototype%</dfn>.</li>
<li>is the intrinsic object <dfn>%Number.prototype%</dfn>.</li>
Copy link
Member

Choose a reason for hiding this comment

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

same here, and throughout.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Aug 25, 2019

Force-pushed the branch. The only new things are the last two commits, which should resolve recent discussions here with @ljharb.

@ljharb ljharb requested a review from a team August 29, 2019 17:25
jmdyck added a commit to jmdyck/ecma262 that referenced this pull request Sep 9, 2019
Commit f1b22ef (tc39#1376) changed lots of intrinsic-names
from old-style to new-style.

But these two should have stayed old-style,
so switch them back.

(See tc39#1636 (comment))
@jmdyck
Copy link
Collaborator Author

jmdyck commented Sep 16, 2019

Added a commit to fix a step-reference in Annex B.

@jmdyck
Copy link
Collaborator Author

jmdyck commented Sep 16, 2019

If nothing else, I'd really appreciate the first commit being merged. That one missing dot is very annoying: I'm forever inserting it to get the algorithm to parse, and then removing it so that I don't commit it on some completely unrelated branch.

@ljharb ljharb self-assigned this Sep 16, 2019
ljharb added a commit to jmdyck/ecma262 that referenced this pull request Sep 16, 2019
The ID of this section was changed, but the previous ID was not retained
in "oldids".
In PR tc39#1571:

https://github.com/tc39/ecma262/pull/1571/files#diff-3540caefa502006d8a33cb1385720803L19940
removed the step
"Assert: _constructorInfo_ is not an abrupt completion."
but didn't insert the equivalent "!" on the previous line.

https://github.com/tc39/ecma262/pull/1571/files#diff-3540caefa502006d8a33cb1385720803L23362
removed a ReturnIfAbrupt step,
but didn't insert the equivalent '?' in the previous line.

---

Commit f1b22ef (tc39#1376) changed 2 occurrences of
  "%AsyncIteratorPrototype%
to
  "%AsyncIterator.prototype%
but the latter assumes a well-known intrinsic named "%AsyncIterator%",
which doesn't exist.

This commit changes them back.

---

Commit f1b22ef (tc39#1376) changed lots of intrinsic-names
from old-style to new-style.

But these two should have stayed old-style,
so switch them back.

(See tc39#1636 (comment))
In Editorial commit f1b22ef (tc39#1376):

- "%<var>NativeError</var>Prototype%" changed to "%NativeErrorPrototype%",
  which doesn't make sense, because there's no such intrinsic.

- "%<var>TypedArray</var>Prototype%" changed to "%TypedArrayPrototype%",
  which would be a change in semantics, which shouldn't happen in an Editorial commit.

So these changes were presumably mistakes.
Commit f1b22ef (tc39#1376) changed lots of intrinsic-references
from old-style to new-style. This commit changes a few more.
Commit ea16790 inserted a step,
pushing the Annex B 'insertion point'
from step 28 to step 29.
@ljharb ljharb merged commit 0a5db75 into tc39:master Sep 16, 2019
@jmdyck
Copy link
Collaborator Author

jmdyck commented Sep 16, 2019

Thanks for the merge!

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.

3 participants