Hi Peter,
Thanks a lot for the patch.
In addition to the in file comments it would be great if we could add tests
to
test/mjsunit/object-define-property.js that exercises all the possible paths
(i.e., that this works in all combinations (accessor with only a setter,
accessor with only a getter, accessor with bot getter and setter, data
property
combined with the different descriptors that can be given (combinations of
enumerable, writable, configurable)).
I know that this a bit of work, but some of the many existing tests in
object-define-property have given early warnings of bugs before when
changing
various parts of the property functionality in objects.cc and runtime.cc)
http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc
File src/runtime.cc (right):
http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc#newcode3496
src/runtime.cc:3496: // Implements part of 8.12.9 DefinePropertyAccessor
DefinePropertyAccessor -> DefineOwnProperty
http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc#newcode3498
src/runtime.cc:3498: // Step 4b - defining a new accessor property
Period at end of the comment + below and above
http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc#newcode3533
src/runtime.cc:3533: // Implements part of 8.12.9 DefineOwnProperty
Period at end of comment + below
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js
File src/v8natives.js (right):
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode550
src/v8natives.js:550: if ((IsGenericDescriptor(desc) ||
Remove space at the end of the line
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode568
src/v8natives.js:568: if (desc.isConfigurable() ||
Remove space at the end of the line
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode569
src/v8natives.js:569: (desc.hasEnumerable() &&
Remove space at the end of the line
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode618
src/v8natives.js:618: if (IsDataDescriptor(desc) ||
Remove space at the end of the line
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode619
src/v8natives.js:619: (IsGenericDescriptor(desc) &&
Remove space at the end of the line
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode622
src/v8natives.js:622: // Step 4a - defining a new data property
Period at end of comment + below
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode623
src/v8natives.js:623: // Steps 9b & 12 - replacing an existing accessor
property with a data
Remove space at the end of the line
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode642
src/v8natives.js:642:
Remove spaces from the empty line
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode644
src/v8natives.js:644: } else {
How about:
} else if (IsGenericDescriptor(desc)){
// Update a genereic accessor with a generic descriptor.
%DefineOrRedefineAccessorProperty(obj, p, GETTER, current.getGet(),
flag);
} else {
// There are 3 cases that lead here:
// Step 4b - defining a new accessor property.
// Steps 9c & 12 - replacing an existing data property with an
accessor
// property.
// Step 12 - updating an existing accessor property with an
accessor.
if (desc.hasGetter()) {
%DefineOrRedefineAccessorProperty(obj, p, GETTER, desc.getGet(),
flag);
}
if (desc.hasSetter()) {
%DefineOrRedefineAccessorProperty(obj, p, SETTER, desc.getSet(),
flag);
}
}
This saves us an extra call to runtime in case a generic descriptor is
passed or if we are defining or redefining either just the setter or
just the getter
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode646
src/v8natives.js:646: // Step 4b - defining a new accessor property
period at the end of the comment + below
http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-property.js
File test/mjsunit/object-define-property.js (right):
http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-property.js#newcode765
test/mjsunit/object-define-property.js:765: var descAccessor = { get:
function() { return 0; } };
could we also try with descElementNonWritable, which should also throw
http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-property.js#newcode855
test/mjsunit/object-define-property.js:855: var descAccessor = { get:
function() { return 0; } };
Again, could we also try with descElementNonWritable and
descNonEnumerable which should both throw
http://codereview.chromium.org/6035014/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
