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

Don't recurse in typed array constructor #269

Closed
wants to merge 3 commits into from

Conversation

anba
Copy link
Contributor

@anba anba commented Dec 21, 2015

No description provided.

@anba
Copy link
Contributor Author

anba commented Dec 21, 2015

The recursion happens in this case:

class MyInt8 extends Int8Array {
  constructor(...args) {
    print("MyInt8 called with:", args);
    super(...args);
  }
}
// Prints:
// MyInt8 called with: 1,2,3
// MyInt8 called with: 3
new MyInt8([1, 2, 3]);

</emu-alg>

<!-- es6num="22.2.2.1.1" -->
<emu-clause id="sec-typedarrayfrom" aoid="TypedArrayFrom">
<h1>Runtime Semantics: TypedArrayFrom( _constructor_, _items_, _mapfn_, _thisArg_ )</h1>
<p>When the TypedArrayFrom abstract operation is called with arguments _constructor_, _items_, _mapfn_, and _thisArg_, the following steps are taken:</p>
<h1>Runtime Semantics: TypedArrayFrom( _O_, _constructor_, _items_, _mapfn_, _thisArg_ )</h1>
Copy link
Member

Choose a reason for hiding this comment

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

The signature of this AO is a bit strange now, since you either pass the first argument or the second. Is there some refactoring that needs to be done to clean this up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just reintroduced the strangeness. 😛

(A similar signature was present in ES6 rev24-rev30, except the O and constructor parameters are switched now.)

Let me think about this a bit, maybe it can be improved...

@bterlson
Copy link
Member

The various ways of creating TAs are getting hard to follow. Perhaps a high level overview of the available AOs and how/when to use them is in order?

@anba
Copy link
Contributor Author

anba commented Dec 21, 2015

Added two new commits:

  1. 31bfc5a
    • Moves the common ArrayBuffer allocation to a new abstract operation (AllocateTypedArrayBuffer).
    • Adds descriptions to TypedArrayCreate and TypedArraySpeciesCreate (text copied more or less copied from ArraySpeciesCreate)
  2. 3e33231
    • Removes the TypedArrayFrom abstraction in favour of simpler algorithms
    • (A bit more work is required in implementations which want to avoid allocating the temporary array object created in IterableToArrayLike. In self-hosted environments this may not be an issue, e.g. V8 works quite similar to the proposed changes.)

@anba
Copy link
Contributor Author

anba commented Dec 22, 2015

WDYT about changing TypedArrayCreate and TypedArraySpeciesCreate to accept an optional minimumLength parameter and use that one instead of the "If argumentList is a List of a single Number, ..." condition in TypedArrayCreate? Will this improve the current situation?

<emu-clause id="typedarray-create" aoid="TypedArrayCreate">
  <h1>TypedArrayCreate( _constructor_, _argumentList_ [ , _minimumLength_ ] )</h1>
  <p>The abstract operation TypedArrayCreate with arguments _constructor_ and _argumentList_ is used to specify the creation of a new TypedArray object using a constructor function. The optional argument _minimumLength_ (a positive integer) is used to validate the newly created TypedArray object's array length. TypedArrayCreate performs the following steps:</p>
  <emu-alg>
    1. Let _newTypedArray_ be ? Construct(_constructor_, _argumentList_).
    1. Perform ? ValidateTypedArray(_newTypedArray_).
    1. If _minimumLength_ is present,
      1. Assert: _minimumLength_ is an integer Number &ge; 0.
      1. If the value of _newTypedArray_'s [[ArrayLength]] internal slot < _minimumLength_, throw a *TypeError* exception.
    1. Return _newTypedArray_.
  </emu-alg>
</emu-clause>

<emu-clause id="typedarray-species-create" aoid="TypedArraySpeciesCreate">
  <h1>TypedArraySpeciesCreate( _exemplar_, _argumentList_ [ , _minimumLength_ ] )</h1>
  <p>The abstract operation TypedArraySpeciesCreate with arguments _exemplar_, _argumentList_, and optionally _minimumLength_ is used to specify the creation of a new TypedArray object using a constructor function that is derived from _exemplar_. It performs the following steps:</p>
  <emu-alg>
    1. Assert: _exemplar_ is an Object that has a [[TypedArrayName]] internal slot.
    1. Let _defaultConstructor_ be the intrinsic object listed in column one of <emu-xref href="#table-49"></emu-xref> for the value of _exemplar_'s [[TypedArrayName]] internal slot.
    1. Let _constructor_ be ? SpeciesConstructor(_exemplar_, _defaultConstructor_).
    1. Return ? TypedArrayCreate(_constructor_, _argumentList_). If _minimumLength_ was passed as a parameter then pass its value as the _minimumLength_ optional argument of TypedArrayCreate.
  </emu-alg>
</emu-clause>

@bterlson
Copy link
Member

@anba this change looks great. Really good catch on the mistakenly added recursiveness and I think the refactorings you've made help clarify this text a lot.

I am also a little uncomofrtable with the "List of a single Number" step but I don't feel strongly enough about it that I think we should add an additional parameter (and pass it appropriately for (admittedly few) calls) to avoid it. That said, I leave it up to you if you want to add it.

@bterlson
Copy link
Member

I also rebased this with latest master. There were some conflicts. If you would like to review this, the commit is here: bterlson@eadbf0b.

@bterlson bterlson closed this in 788a2eb Dec 30, 2015
@anba
Copy link
Contributor Author

anba commented Jan 11, 2016

@anba this change looks great. Really good catch on the mistakenly added recursiveness and I think the refactorings you've made help clarify this text a lot.

Thanks!

I am also a little uncomofrtable with the "List of a single Number" step but I don't feel strongly enough about it that I think we should add an additional parameter (and pass it appropriately for (admittedly few) calls) to avoid it.

That's also my opinion on this. So let's just defer this change for now.

There were some conflicts. If you would like to review this, the commit is here: bterlson/ecma262@eadbf0b.

Looks good, except a pre-existing bug in my PR (step 4 in sec-typedarray-buffer-byteoffset-length has </code>%<var>TypedArray instead of <code>%<var>TypedArray).

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.

3 participants