Skip to content
This repository has been archived by the owner on Sep 8, 2021. It is now read-only.

Editor review feedback #31

Merged
merged 4 commits into from
Jan 15, 2021
Merged

Editor review feedback #31

merged 4 commits into from
Jan 15, 2021

Conversation

rbuckton
Copy link
Collaborator

Address early editor review feedback from #22.

@rbuckton rbuckton added this to the Stage 3 milestone Jan 13, 2021
@rbuckton rbuckton requested a review from ljharb January 13, 2021 23:48
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

seems reasonable

@rbuckton rbuckton requested a review from syg January 14, 2021 20:35
1. Set _env_.[[FunctionObject]] to _F_.
1. Set _env_.[[ThisBindingStatus]] to ~uninitialized~.
1. Let _home_ be _F_.[[HomeObject]].
1. Set _env_.[[HomeObject]] to _home_.
Copy link
Contributor

Choose a reason for hiding this comment

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

The above two steps could be coalesced.

<h1>Environment Record Operations</h1>
<emu-clause id="sec-newclassstaticblockenvironment" aoid="NewClassStaticBlockEnvironment">
<h1><ins>NewClassStaticBlockEnvironment ( _F_ )</ins></h1>
<p>The abstract operation NewClassStaticBlockEnvironment takes arguments _F_. It performs the following steps when called:</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p>The abstract operation NewClassStaticBlockEnvironment takes arguments _F_. It performs the following steps when called:</p>
<p>The abstract operation NewClassStaticBlockEnvironment takes argument _F_ (a function object). It performs the following steps when called:</p>

Copy link
Collaborator Author

@rbuckton rbuckton Jan 15, 2021

Choose a reason for hiding this comment

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

We aren't very clear in the spec how to specify the types of arguments. For some cases we use (a function object), while for others we use 1. Assert: _F_ is an ECMAScript function., but we generally don't combine the two. I have the function assertion as the first line of the algorithm.

Note that this is based on https://tc39.es/ecma262/#sec-newfunctionenvironment, which uses only the assertion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is true, it is not great to give a more general type and then assert that it is a more specific type in the assert, even if it's not incorrect in how it's used.

@@ -243,7 +235,7 @@ <h1><ins>Runtime Semantics: ClassStaticBlockEvaluation</ins></h1>
1. Let _innerContext_ be PrepareForClassStaticBlockEvaluation(_F_).
1. Assert: _innerContext_ is now the running execution context.
1. Perform ClassStaticBlockBindThis(_F_, _innerContext_).
1. Let _result_ be ClassStaticBlockEvaluateBody(_F_).
1. Let _result_ be ClassStaticBlockEvaluateBody(|ClassStaticBlockBody|).
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit weird, but we can address it later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How is this weird? We call AOs with parse nodes in other places in the spec. For example, the use of IsAnonymousFunctionDefinition here:

https://tc39.es/ecma262/#sec-object-initializer-runtime-semantics-propertydefinitionevaluation

image

Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird in that I'd like a sharper line dividing runtime SDOs and AOs. And agreed that this is an existing oddity, thus no need to act on it now.

@rbuckton rbuckton merged commit 81648e2 into master Jan 15, 2021
@rbuckton rbuckton deleted the editorReview branch January 15, 2021 05:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants