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: Tweak ValidateAndApplyPropertyDescriptor #1614

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@shvaikalesh
Copy link
Contributor

commented Jul 8, 2019

No description provided.

@ljharb

This comment has been minimized.

Copy link
Member

commented Jul 9, 2019

Can you provide more context on why you feel this is an improvement?

@shvaikalesh shvaikalesh force-pushed the shvaikalesh:apply-prop-desc branch from b49dd43 to 9d129f2 Jul 9, 2019

<p>If the initial values of a property's attributes are not explicitly specified by this specification, the default value defined in <emu-xref href="#table-4"></emu-xref> is used.</p>
<emu-table id="table-4" caption="Default Attribute Values">
<p>If the initial values of a property's attributes are not explicitly specified by this specification, the default value defined in <emu-xref href="#table-default-attribute-values"></emu-xref> is used.</p>
<emu-table id="table-default-attribute-values" caption="Default Attribute Values">

This comment has been minimized.

Copy link
@shvaikalesh

shvaikalesh Jul 9, 2019

Author Contributor

So we can link here "default value(s)" references in ValidateAndApplyPropertyDescriptor (and vice versa).

This comment was marked as resolved.

Copy link
@jmdyck

jmdyck Jul 10, 2019

Collaborator
Suggested change
<emu-table id="table-default-attribute-values" caption="Default Attribute Values">
<emu-table id="table-default-attribute-values" caption="Default Attribute Values" oldids="table-4">

since there might be external links to table-4.

This comment was marked as resolved.

Copy link
@shvaikalesh

shvaikalesh Jul 10, 2019

Author Contributor

Perfect, thank you. b575b33

Show resolved Hide resolved spec.html
1. Return *true*.
1. If every field in _Desc_ is absent, return *true*.
1. If _current_.[[Configurable]] is *false*, then
1. If _Desc_.[[Configurable]] is present and its value is *true*, return *false*.
1. If _Desc_.[[Enumerable]] is present and the [[Enumerable]] fields of _current_ and _Desc_ are the Boolean negation of each other, return *false*.
1. If IsGenericDescriptor(_Desc_) is *true*, no further validation is required.

This comment has been minimized.

Copy link
@shvaikalesh

shvaikalesh Jul 9, 2019

Author Contributor

Considering that name of abstract op is ValidateAndApplyPropertyDescriptor, "no further validation is required" may look like "end algorithm here" to infrequent reader.
Let's mark it as note, which it is.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 10, 2019

Member

it might be cleaner to instead nest this entire thing in an "if IsGenericDescriptor is false"?

This comment has been minimized.

Copy link
@shvaikalesh

shvaikalesh Jul 11, 2019

Author Contributor

Personally, I am not a fan of deep nesting.

Another way may be to extract validation logic to distinct abstract op (and early return for generic descriptors), but I don't see how we can do this without adding complexity to this algorithm.

Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html
Show resolved Hide resolved spec.html
@ljharb

ljharb approved these changes Jul 11, 2019

@ljharb ljharb requested review from zenparsing and tc39/ecma262-editors Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.