Strict mode delete.

Strict mode delete.
(includes delete of unqualified identifier)
Pass strict mode flag to Runtime_DeleteProperty and then
via STRICT_DELETION to JSObject::Delete(Property/Element).
Throw TypeError when deleting non configurable property/element.
Adding mozilla test to .gitignore.

BUG=
TEST=test/mjsunit/strict-mode.js

[Martin Maly, 2011-02-14 05:15:22]

The second part of implementation of delete behavior in strict mode (deleting
non-configurable property).

This change builds on the previous (delete of unqualified identifier). As a
result, only "delete o.x" and delete o[x] can happen in strict mode which
explains why sometimes constant kNonStrictMode is passed to the DELETE runtime
function.

Thank you!
Martin

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);
"delete foo" is disallowed in strict mode so we can only reach this code in
non-strict mode.

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#new...
src/arm/full-codegen-arm.cc:3067: __ push(r1);
Is there a preferred register to use instead?

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#...
src/arm/lithium-codegen-arm.cc:3893: __ Push(object, key, strict);
Not sure 100% about the use of scratch0() but saw it elsewhere.

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);
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.

http://codereview.chromium.org/6515005/diff/1/src/objects.h
File src/objects.h (right):

http://codereview.chromium.org/6515005/diff/1/src/objects.h#newcode1293
src/objects.h:1293: };
This enum felt like natural place to capture strict delete. I actually
originally built the code passing around additional parameter and then
consolidated back to this enum, making sure all code paths are handled.

http://codereview.chromium.org/6515005/diff/1/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/6515005/diff/1/src/parser.cc#newcode2522
src/parser.cc:2522: }
This is contained in the other CL...

[Mads Ager (chromium), 2011-02-14 10:27:24]

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#new...
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#new...
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#...
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#new...
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#new...
test/mjsunit/strict-mode.js:270: CheckStrictMode("function function_name() {
delete function_name; }", SyntaxError);
Make these 80 char lines?

[Kevin Millikin (Chromium), 2011-02-14 11:56:55]

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 10:27:24, Mads Ager wrote:
> 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?

It just doesn't rewrite to a property reference (global variables don't have a
rewrite).

[Martin Maly, 2011-02-14 21:46:51]

Incorporated the feedback. Thank you!
Martin

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 10:27:24, Mads Ager wrote:
> 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?

Yes, this rewrites to a slot.
Added comment.

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#new...
src/arm/full-codegen-arm.cc:3072: ASSERT(strict_mode_flag() == kNonStrictMode);
On 2011/02/14 10:27:24, Mads Ager wrote:
> Maybe duplicate the comment here to explain the assert?

Done.

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 10:27:24, Mads Ager wrote:
> 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?
> 

Done.

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#new...
test/mjsunit/strict-mode.js:267: // Delete of an unqialified identifier
On 2011/02/14 10:27:24, Mads Ager wrote:
> unqialified -> unqualified

Done.

http://codereview.chromium.org/6515005/diff/1/test/mjsunit/strict-mode.js#new...
test/mjsunit/strict-mode.js:270: CheckStrictMode("function function_name() {
delete function_name; }", SyntaxError);
On 2011/02/14 10:27:24, Mads Ager wrote:
> Make these 80 char lines?

Done.