LGTM
http://codereview.chromium.org/6515005/diff/1/src/arm/codegen-arm.cc File src/arm/codegen-arm.cc (right): http://codereview.chromium.org/6515005/diff/1/src/arm/codegen-arm.cc#newcode5852 src/arm/codegen-arm.cc:5852: ASSERT(strict_mode_flag() == kNonStrictMode); On 2011/02/14 05:15:22, Martin Maly wrote:
"delete foo" is disallowed in strict mode so we can only reach this
code in
non-strict mode.
And 'this' rewrites to a slot, right? Also, could you put in your review comment here as a code comment to make it clearer when reading this code later? http://codereview.chromium.org/6515005/diff/1/src/arm/full-codegen-arm.cc File src/arm/full-codegen-arm.cc (right): http://codereview.chromium.org/6515005/diff/1/src/arm/full-codegen-arm.cc#newcode3067 src/arm/full-codegen-arm.cc:3067: __ push(r1); On 2011/02/14 05:15:22, Martin Maly wrote:
Is there a preferred register to use instead?
r1 is fine, r0 would be fine too. No preferences here. The only reason why the register choice would matter here is if you need to push more than one value at which point you can use Push to use the stm (store multiple) instruction. Here it is clearer to just visit for stack value for the two first parameters and just push the last one as you are doing. http://codereview.chromium.org/6515005/diff/1/src/arm/full-codegen-arm.cc#newcode3072 src/arm/full-codegen-arm.cc:3072: ASSERT(strict_mode_flag() == kNonStrictMode); Maybe duplicate the comment here to explain the assert? http://codereview.chromium.org/6515005/diff/1/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): http://codereview.chromium.org/6515005/diff/1/src/arm/lithium-codegen-arm.cc#newcode3893 src/arm/lithium-codegen-arm.cc:3893: __ Push(object, key, strict); On 2011/02/14 05:15:22, Martin Maly wrote:
Not sure 100% about the use of scratch0() but saw it elsewhere.
This is fine. scratch0() is always free to use in the arm lithium backend. It is excluded from register allocation. The Push instruction here cannot use stm anyway because object is r0 and key is r1. http://codereview.chromium.org/6515005/diff/1/src/objects.cc File src/objects.cc (right): http://codereview.chromium.org/6515005/diff/1/src/objects.cc#newcode2623 src/objects.cc:2623: Object* result = dictionary->DeleteProperty(entry, mode); On 2011/02/14 05:15:22, Martin Maly wrote:
DeleteProperty is a wrong place to report the error because it is
missing the
necessary data to report good error, and it is called from other
places which
don't even have the relevant information. Checking for false_value() for strict mode delete felt cleanest.
I think that makes sense. Could you add a comment in the body of the if statement that deleting non-configurable properties in strict mode should throw exceptions? http://codereview.chromium.org/6515005/diff/1/test/mjsunit/strict-mode.js File test/mjsunit/strict-mode.js (right): http://codereview.chromium.org/6515005/diff/1/test/mjsunit/strict-mode.js#newcode267 test/mjsunit/strict-mode.js:267: // Delete of an unqialified identifier unqialified -> unqualified http://codereview.chromium.org/6515005/diff/1/test/mjsunit/strict-mode.js#newcode270 test/mjsunit/strict-mode.js:270: CheckStrictMode("function function_name() { delete function_name; }", SyntaxError); Make these 80 char lines? http://codereview.chromium.org/6515005/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
