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 format of *.prototype.sort #1612

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@jmdyck
Copy link
Collaborator

commented Jul 6, 2019

The main point was to to make a single <emu-alg> out of some of the bits and pieces that currently define Array.prototype.sort.

Also, %TypedArray%.prototype.sort's version of SortCompare should have its own name and sub-clause, so I did that.

jmdyck added some commits Jul 6, 2019

Editorial: Factor out ObjectIsSparse operation
... and set _objIsSparse_ metavariable.
Editorial: Rewrite Array.p.sort as a unified algorithm
Formerly, there were 5 spots where we said:
   "the sort order is [also] implementation-defined"
Instead, collect the various conditions into a single list.

And rather than a <ul>, make it an ecmarkdown bullet-list,
which can then be merged with the "entry steps" and the "following steps"
into a single <emu-alg>.

(Note that this is basically just a change in formatting --
there is no change to the normative requirements.)
Editorial: Move two sort order conditions
... to a more logical spot in the list (in Array.p.sort).
Editorial: introduce TypedArraySortCompare
(Give %TypedArray%.prototype.sort's version of SortCompare
its own name and sub-clause.)
@@ -32711,6 +32674,18 @@ <h1>Array.prototype.sort ( _comparefn_ )</h1>
<p>The `sort` function is intentionally generic; it does not require that its *this* value be an Array object. Therefore, it can be transferred to other kinds of objects for use as a method.</p>
</emu-note>

<emu-clause id="sec-objectissparse" aoid="ObjectIsSparse">
<h1>Runtime Semantics: ObjectIsSparse ( _obj_, _len_ )</h1>

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member

this seems more like it should be "ArraylikeIsSparse"?

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jul 7, 2019

Author Collaborator

Maybe.

The spec doesn't actually define "array-like", and I don't think I want to contribute to the suggestion that it does. It'd be different if we were to define it...

Based on the evidence (in CreateListFromArrayLike and the preamble of ToLength), it seems like the only requirement on an array-like object is that it have a "length" property whose value is (or can be converted by various means to) an integer in the range [0, 2^53 - 1]. Or more specifically, that ToLength(? Get(obj, "length"))
return an integer rather than an abrupt completion. (There's also an expectation that it will have some integer index properties, though that presumably isn't a requirement, i.e. you can have an 'empty' array-like.)

Assuming that's the definition, the IsSparse operation doesn't require that its _obj_ be array-like (since it doesn't care where _len_ came from), but on the other hand, the spec will only call the operation with an array-like object. Maybe the operation should only have the _obj_ parameter, and 'recalculate' _len_. That would make it more self-contained and more deserving of "ArrayLike" in the name.

Maybe there should be a GetLengthOfArrayLike operation, and maybe that should be used wherever an array-like is required. (E.g., that would help convey the generic-ness of Array.prototype.*.) That (and defining "array-like") sounds like a separate PR though.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member

I’d consider it already defined based on the abstract operation you mentioned, just not explicitly.

</ul>
<p>The following steps are taken:</p>
<emu-alg>
1. Let _objIsSparse_ be ObjectIsSparse(_obj_, _len_).

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member
Suggested change
1. Let _objIsSparse_ be ObjectIsSparse(_obj_, _len_).
1. Let _objIsSparse_ be ? ObjectIsSparse(_obj_, _len_).

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jul 7, 2019

Author Collaborator

Yup.

<p>The following steps are taken:</p>
<emu-alg>
1. Let _objIsSparse_ be ObjectIsSparse(_obj_, _len_).
1. Let _proto_ be _obj_.[[GetPrototypeOf]]().

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member

? or !?

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jul 7, 2019

Author Collaborator

?, presumably. I don't think there's anything here to guarantee a normal return.

1. Let _sortOrderIsImplementationDefined_ be *true* if any of the following conditions are satisfied, and let it be *false* otherwise:
* _objIsSparse_ is *true* and _proto_ is not *null* and there exists a nonnegative integer _j_ less than _len_ such that HasProperty(_proto_, ToString(_j_)) is *true*.
* _objIsSparse_ is *true* and IsExtensible(_obj_) is *false*.
* _objIsSparse_ is *true* and any integer index property of _obj_ whose name is a nonnegative integer less than _len_ is a data property whose [[Configurable]] attribute is *false*.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member

this seems like it needs to be specified in terms of a MOP operation

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jul 7, 2019

Author Collaborator

It's the same wording as the status quo, so it seems like it shouldn't be this PR's problem to resolve.

But yeah, I think writing it properly would involve _obj_.[[GetOwnProperty]]

1. Let _proto_ be _obj_.[[GetPrototypeOf]]().
1. Let _sortOrderIsImplementationDefined_ be *true* if any of the following conditions are satisfied, and let it be *false* otherwise:
* _objIsSparse_ is *true* and _proto_ is not *null* and there exists a nonnegative integer _j_ less than _len_ such that HasProperty(_proto_, ToString(_j_)) is *true*.
* _objIsSparse_ is *true* and IsExtensible(_obj_) is *false*.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member
Suggested change
* _objIsSparse_ is *true* and IsExtensible(_obj_) is *false*.
* _objIsSparse_ is *true* and ! IsExtensible(_obj_) is *false*.

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jul 7, 2019

Author Collaborator

Hm, I would have thought ?.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 8, 2019

Member

if it can indeed return abruptly here then use ?, without the explicit notation its harder to be sure :-)

1. Let _objIsSparse_ be ObjectIsSparse(_obj_, _len_).
1. Let _proto_ be _obj_.[[GetPrototypeOf]]().
1. Let _sortOrderIsImplementationDefined_ be *true* if any of the following conditions are satisfied, and let it be *false* otherwise:
* _objIsSparse_ is *true* and _proto_ is not *null* and there exists a nonnegative integer _j_ less than _len_ such that HasProperty(_proto_, ToString(_j_)) is *true*.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member
Suggested change
* _objIsSparse_ is *true* and _proto_ is not *null* and there exists a nonnegative integer _j_ less than _len_ such that HasProperty(_proto_, ToString(_j_)) is *true*.
* _objIsSparse_ is *true* and _proto_ is not *null* and there exists a nonnegative integer _j_ less than _len_ such that ! HasProperty(_proto_, ! ToString(_j_)) is *true*.

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jul 7, 2019

Author Collaborator

Yes to the ! for ToString. As for HasProperty, I think that has to be ?.

But now I'm wondering: with the status quo, if HasProperty (or IsExtensible, or various others) returns an abrupt completion, does that cause sort to return an abrupt completion, or does that merely cause the condition in question to not be satisfied, which then only affects whether the sort order is implementation-defined? (Note that these conditions are outside the "Perform a sequence of calls" step, so its point about abrupt completions doesn't apply.)

* _objIsSparse_ is *true* and IsExtensible(_obj_) is *false*.
* _objIsSparse_ is *true* and any integer index property of _obj_ whose name is a nonnegative integer less than _len_ is a data property whose [[Configurable]] attribute is *false*.
* _obj_ is an exotic object (including Proxy exotic objects) whose behaviour for [[Get]], [[Set]], [[Delete]], and [[GetOwnProperty]] is not the ordinary object implementation of these internal methods.
* any index property of _obj_ whose name is a nonnegative integer less than _len_ is an accessor property or is a data property whose [[Writable]] attribute is *false*.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member

can this be written in terms of abstract operations?

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jul 7, 2019

Author Collaborator

If you mean current abstract operations, I don't think so. If you mean, could we create an abstract operation for this purpose, then of course.

1. Assert: _obj_ is an Object.
1. Assert: _len_ is a nonnegative integer.
1. For each integer _i_ in the range 0 &le; _i_ &lt; _len_, do
1. Let _elem_ be _obj_.[[GetOwnProperty]](! ToString(_i_)).

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member
Suggested change
1. Let _elem_ be _obj_.[[GetOwnProperty]](! ToString(_i_)).
1. Let _elem_ be ? _obj_.[[GetOwnProperty]](! ToString(_i_)).

because i believe this can throw on, for example, an uninitialized module namespace object?

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jul 7, 2019

Author Collaborator

A module namespace object probably doesn't have a "length" property, so it wouldn't get this far. But in general, yes, [[GetOwnProperty]] could throw.

<h1>Runtime Semantics: TypedArraySortCompare ( _x_, _y_ )</h1>
<p>The TypedArraySortCompare abstract operation is called with two arguments _x_ and _y_. It also has access to the _comparefn_ and _buffer_ values of the current invocation of the %TypedArray%`.prototype.sort` method. It performs a numeric comparison rather than the string comparison used in <emu-xref href="#sec-array.prototype.sort"></emu-xref>. The following steps are taken:</p>
<emu-alg>
1. Assert: Both Type(_x_) and Type(_y_) is Number.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member
Suggested change
1. Assert: Both Type(_x_) and Type(_y_) is Number.
1. Assert: both Type(_x_) and Type(_y_) are Number.

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jul 7, 2019

Author Collaborator

(Note that the <emu-alg> text is the same as in the current spec -- github is only showing it as different because I had to change the indentation.)

re lowercase 'b': The spec is pretty consistent in capitalizing the first word after "Assert:".

re "is" to "are": That would be better, but I think I'd prefer Assert: Type(_x_) is Number and Type(_y_) is Number

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member

Sounds good on both points.

1. Assert: Both Type(_x_) and Type(_y_) is Number.
1. If _comparefn_ is not *undefined*, then
1. Let _v_ be ? ToNumber(? Call(_comparefn_, *undefined*, &laquo; _x_, _y_ &raquo;)).
1. If IsDetachedBuffer(_buffer_) is *true*, throw a *TypeError* exception.

This comment has been minimized.

Copy link
@ljharb

ljharb Jul 7, 2019

Member
Suggested change
1. If IsDetachedBuffer(_buffer_) is *true*, throw a *TypeError* exception.
1. If ! IsDetachedBuffer(_buffer_) is *true*, throw a *TypeError* exception.

This comment has been minimized.

Copy link
@jmdyck

jmdyck Jul 7, 2019

Author Collaborator

If you insist.

@ljharb ljharb requested a review from mathiasbynens Jul 7, 2019

@mathiasbynens

This comment has been minimized.

Copy link
Member

commented Jul 8, 2019

(Note that #1585 is up for discussion at this month's TC39 meeting. Depending on the outcome, we may or may not have a merge conflict.)

@jmdyck

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2019

Ah, should have checked the agenda. So we might as well pause this PR until that's decided.

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.