LGTM
http://codereview.chromium.org/2278002/diff/7001/8001 File src/runtime.cc (right): http://codereview.chromium.org/2278002/diff/7001/8001#newcode606 src/runtime.cc:606: elms->set(0, Heap::false_value()); On 2010/05/26 13:43:57, Rico wrote:
On 2010/05/26 11:23:18, Mads Ager wrote: > Maybe we should define named constants for 0-4. Would make it easier
to read
the > code for property descriptors. They problem is that they represent different things depending on if
it is a
data property or an accessor as described by the method comment. I
could make
two enums or one with mixed names (i.e., GET_FUNCTION_OR_VALUE and SET_FUNCTION_OR_WRITABLE). We would still, however, have magic numbers
in
v8natives.js.
More than one enum would be fine by me. Also, you can use macros.py to avoid magic numbers in v8natives.js. Feel free to do that as a followup change though, but it would be nice for readability. http://codereview.chromium.org/2278002/diff/8003/12001#newcode597 src/runtime.cc:597: This is by -> This is handled by? http://codereview.chromium.org/2278002/diff/8003/12002 File test/mjsunit/get-own-property-descriptor.js (right): http://codereview.chromium.org/2278002/diff/8003/12002#newcode33 test/mjsunit/get-own-property-descriptor.js:33: function get() {return x;} Could you add a bit of spacing here? { return x; } http://codereview.chromium.org/2278002/diff/14001/15002#newcode28 test/mjsunit/get-own-property-descriptor.js:28: // This file only tests very simple descriptors that always have Could you add testing of objects that have the element in their prototype chain instead of on the object itself? There was a code change to use HasLocalElement instead of HasElement. We should make sure there is a regression test for that. http://codereview.chromium.org/2278002/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
