Thanks caitp for your comments, I think they are very valid and I also have
doubts. I comment below.
On 2014/08/05 02:09:07, caitp wrote:
I'm not an owner, so take this with a grain of salt:
https://codereview.chromium.org/364853009/diff/260001/src/harmony-array.js
File src/harmony-array.js (right):
https://codereview.chromium.org/364853009/diff/260001/src/harmony-array.js#newcode133
src/harmony-array.js:133: %AddElement(array, i, %_Arguments(i), NONE);
My review doesn't mean much here, but I'm not sure I see why we need
Runtime_AddElement:
1. It seems to be a wrapper for CreateDataProperty except that we limit
the
attributes for the property to enumerable (or not), writable (or not), and
whatever we get from DONT_DELETE. Reading it, it doesn't look like we
throw if
we can't successfully create the data property.
The CreateDataProperty spec says that the PropertyDescriptor should be
{writable: true, enumerable: true and cofigurable: true}. That translates to
attributes "None" in V8. I let AddElement receive customized attributes, and
pass "None" in the JavaScript call. I can change the implementation of
AddElement and set the attributes always to "None" so it's a more equivalent
implementation to CreateDataProperty.
My understanding is that DONT_DELETE maps to "configurable: false"
according to
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty
(configurable: true if and only if the type of this property descriptor
may be
changed and if the property may be deleted from the corresponding object).
The spec says that if the property could not be successfully created a
TypeError
exception is launched. More precisely, the spec describes the following
steps:
3. Let success be CreateDataProperty(O, P, V).
4. ReturnIfAbrupt(success).
5. If success is false, then throw a TypeError exception.
I followed the implementation of similar methods. For instance, the spec for
DeletePropertyOrThrow describes these steps:
3. Let success be the result of calling the Delete internal method of O
passing
P as the argument.
4. ReturnIfAbrupt(success).
5. If success is false, then throw a TypeError exception.
However, the implementation of Runtime_DeleteProperty doesn't launch a
TypeError exception, it ends with a call to
"ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, result,
JSReceiver::DeleteProperty(object, key, delete_mode));".
My understanding is that wrapping the JSObject::SetElement call within
ASSIGN_RETURN_FAILURE_ON_EXCEPTION is enough to cover these steps, but
maybe I'm
wrong.
2. It's not totally clear to me what the authors of the spec actually
want,
because it's written to be as obtuse as possible, apparently. But at a
glance
it
looks like what we actually want to do is try to define the property with
the
default property descriptor flags, and throw if we can't. (This might be
entirely wrong, and I would be frankly surprised if anyone, including the
authors of the draft, can walk away from this writing and come back after
2
weeks correctly interpreting what they've written).
I agree, apparently it seems that a property definition (DEFINE_PROPERTY
flag in
V8) is required, in contrast to an array assignment which is generally
described in the spec as "Put(O, Pk, value, true)" (see for instance
Array.prototype.fill). An array assignment in V8 is generally coded as
"JSObject::SetElement(js_object, index, value, attributes, strict_mode,
check_prototype, SET_PROPERTY)".
Before AddProperty was renamed and recoded to AddNamedProperty, AddProperty
could be used to define properties in an array using indexes. This is no
longer
possible. So doing a recap:
* The spec requires to be able to define a property into an indexed
array.
* An indexed array assignment uses the SET_PROPERTY flag in V8 not the
DEFINE_PROPERTY flag.
* It's necessary to provide a method that can define properties into an
indexed array, that means, an operation that does a JSObject::SetElement
using
the DEFINE_PROPERTY flag. This method is AddElement.
This also has to do with one of the tests I imported from Firefox (Array.of
does
not trigger prototype setters). If the implementation of ArrayOf used array
assignment the setter will be triggered. If the array is created defining
properties the setter is not triggered.
3. I don't really see how Runtime_AddElement really provides any of the
abstract
methods described in the draft, and having that clearly explained in
comments
would be really, really valuable. I can't look at this implementation, and
then
look at the spec, and make the connection. I'm just not seeing it. It's
not
that
I don't trust that it's correct (since rossberg@ has already vetted it),
but
I'd
prefer it if the correctness of it to be explained in the tree, in comment
form,
if not by matching the language in the draft.
OK, maybe we could rename the method to match the one from the
specification. I
think what I mentioned in 2) explains what's the rationale behind
AddElement and
how it is related to CreateDataPropertyOrThrow.
I would be very happy if, at the very least, this runtime function, and
how it
is accomplishing what is asked for in 22.1.2.3.8c, were explained clearly
in
comments.
OK, I can add some comments in the header explaining how is related to
CreateDataPropertyOrThrow and for what cases is used (define a data
property in
an indexed array).
Also, nit: the indentation in ArrayOf is inconsistent with the rest of v8
(despite being consistent with Blink and Chromium). I don't care a whole
lot,
but it is what it is.
Correct. Thanks for the pointer. I will change that.
https://codereview.chromium.org/364853009/
--
--
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.
