The new version should fix the issue with properties defined wrong. Is there
anything else that should be fixed?
https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js
File src/harmony-array.js (right):
https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcode197
src/harmony-array.js:197: if (constructor === GlobalArray) {
On 2015/06/11 15:35:33, Toon Verwaest wrote:
On 2015/06/11 15:12:10, adamk wrote:
> On 2015/06/11 13:09:23, Toon Verwaest wrote:
> > On 2015/06/11 13:04:01, arv wrote:
> > > On 2015/06/11 08:03:14, Toon Verwaest wrote:
> > > > Is this enough? A constructor could return an array with
predefined
> > > > getters/setters...
> > >
> > > I believe this is fine.
> > >
> > > constructor here is the receiver in Array.from. The array passed
in here
is
> a
> > > new instance created from Array which will never have own
accessors.
> >
> > > function f() { var result = []; Object.defineProperty(result,
"0",
> > {value:100}); return result; }
> > > Array.from.call(f, [200])
> > (d8):1: illegal access
>
> Do you have Dan's patch patched in? This should go down the slow
path because
f
> !== GlobalArray.
Arg, ok, I see. Nevermind ;)
However, shouldn't we make sure that if this is not a constructor, we
also pass
in GlobalArray?
I think GlobalArray will remain a constructor, so we'll go down the else
path if a non-constructor is passed in.
https://codereview.chromium.org/1181623003/diff/1/src/harmony-array.js#newcode200
src/harmony-array.js:200: ObjectDefineProperty(array, i, { value: value
});
On 2015/06/11 13:04:01, arv wrote:
non enum? non writable? non configurable?
We really should expose something faster than using this high level
API that
uses property descriptors.
How about we provide a function called CreateDataPropertyOrThrow or
CreateDataProperty?
We could also skip the descriptor and use the bit mask we are using
internally
(NONE, DONT_ENUM etc)
The refactoring can be done later but the descriptor (enum, writable,
configurable) needs to be fixed and tested.
Fixed the descriptor and added tests.
https://codereview.chromium.org/1181623003/diff/1/test/mjsunit/harmony/array-from.js
File test/mjsunit/harmony/array-from.js (right):
https://codereview.chromium.org/1181623003/diff/1/test/mjsunit/harmony/array-from.js#newcode170
test/mjsunit/harmony/array-from.js:170: })();
On 2015/06/11 13:04:01, arv wrote:
I see that there is already a test for accessors on Array.prototype.
Not sure what you're referring to, but I added a test.
https://codereview.chromium.org/1181623003/
--
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
