Thanks for the code review Rico.
This update addresses all the in file comments. I added the 2 test cases
that
you mentioned in the in file comments, but they don't throw. This was a
surprise
to me as well, but the spec is clear. I confirmed with an email thread to
es-discuss. I've attached the thread below.
I agree that more tests in object-define-property.js would be a good thing.
I'm
going on vacation in a couple of hours so I thought I'd upload what I have.
I'll
resume this when I'm back next week.
Cheers,
Peter
From es-discuss:
On Wed, Jan 5, 2011 at 2:09 PM, Allen Wirfs-Brock <[email protected]>
wrote:
Yes, it is intentional and was allowed to enable the "freezing" of objects
that
already had non-configurable properties. MarkM can probably provide some
specific use cases in the context of secure sandboxes.
Follow up from MarkM:
According to the ES5 spec (I haven't double checked against ES5.1), the
following properties are initialized to {writable: true, configurable:
false}:
13.2 step 18, 15.3.5.2
aFunction.prototype
15.4.5.2
anArray.length
15.10.7.5
aRegExp.lastIndex
However, we need to be able to freeze functions, arrays, and regExps. Were
we
willing to make the above properties configurable: true, then we wouldn't
have
needed this loophole in the spec. Given that we were not willing to make
these
properties configurable, this loophole was the least unpleasant way we could
find to still allow us to freeze such objects. (Another approach that was
mentioned at the time and promptly rejected: introducing yet another
attribute
flag to distinguish not-deletable-but-freezable from non-configurable.)
I think there were some other cases as well, but can't find them right now.
No
matter, the above cases are sufficient.
In addition, if I read the spec right, the following code
15.5.5.2
Object.getOwnPropertyDescriptor(Object("foo"), '1')
should return a descriptor claiming in effect that the (non-existent) '1'
property on the String object is writable but not configurable. Is this a
spec
error? Is it already recorded as an errata? Cc'ing es5-discuss just in case.
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
On 2011/01/05 13:37:01, Rico wrote:
DefinePropertyAccessor -> DefineOwnProperty
Done.
http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc#newcode3498
src/runtime.cc:3498: // Step 4b - defining a new accessor property
On 2011/01/05 13:37:01, Rico wrote:
Period at end of the comment + below and above
Done.
http://codereview.chromium.org/6035014/diff/2001/src/runtime.cc#newcode3533
src/runtime.cc:3533: // Implements part of 8.12.9 DefineOwnProperty
On 2011/01/05 13:37:01, Rico wrote:
Period at end of comment + below
Done.
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) ||
On 2011/01/05 13:37:01, Rico wrote:
Remove space at the end of the line
Done.
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode568
src/v8natives.js:568: if (desc.isConfigurable() ||
On 2011/01/05 13:37:01, Rico wrote:
Remove space at the end of the line
Done.
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode569
src/v8natives.js:569: (desc.hasEnumerable() &&
On 2011/01/05 13:37:01, Rico wrote:
Remove space at the end of the line
Done.
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode618
src/v8natives.js:618: if (IsDataDescriptor(desc) ||
On 2011/01/05 13:37:01, Rico wrote:
Remove space at the end of the line
Done.
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode619
src/v8natives.js:619: (IsGenericDescriptor(desc) &&
On 2011/01/05 13:37:01, Rico wrote:
Remove space at the end of the line
Done.
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode622
src/v8natives.js:622: // Step 4a - defining a new data property
On 2011/01/05 13:37:01, Rico wrote:
Period at end of comment + below
Done.
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
On 2011/01/05 13:37:01, Rico wrote:
Remove space at the end of the line
Done.
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode642
src/v8natives.js:642:
On 2011/01/05 13:37:01, Rico wrote:
Remove spaces from the empty line
Done.
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode644
src/v8natives.js:644: } else {
On 2011/01/05 13:37:01, Rico wrote:
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
Done.
http://codereview.chromium.org/6035014/diff/2001/src/v8natives.js#newcode646
src/v8natives.js:646: // Step 4b - defining a new accessor property
On 2011/01/05 13:37:01, Rico wrote:
period at the end of the comment + below
Done.
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#newcode753
test/mjsunit/object-define-property.js:753: // configurable property
On 2011/01/05 13:40:39, Rico wrote:
period at end of comment
Done.
http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-property.js#newcode763
test/mjsunit/object-define-property.js:763: // the non configurable
property.
On 2011/01/05 13:40:39, Rico wrote:
a the -> a
Done.
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; } };
I've added a test for descElementNonWritable. Interestingly enough it
does not throw. I confirmed with a thread on es-discuss.
On 2011/01/05 13:37:01, Rico wrote:
could we also try with descElementNonWritable, which should also throw
http://codereview.chromium.org/6035014/diff/2001/test/mjsunit/object-define-property.js#newcode843
test/mjsunit/object-define-property.js:843: // configurable property of
an array
On 2011/01/05 13:40:39, Rico wrote:
period at end of comment
Done.
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; } };
On 2011/01/05 13:37:01, Rico wrote:
Again, could we also try with descElementNonWritable and
descNonEnumerable which
should both throw
Done. See comment above.
http://codereview.chromium.org/6035014/
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
