Fix a bug that prevents constants from overwriting function values in object ...

Fix a bug that prevents constants from overwriting function values in object literals. 
BUG=http://code.google.com/p/v8/issues/detail?id=907


Committed: http://code.google.com/p/v8/source/detail?r=5718

[Mads Ager (chromium), 2010-10-26 08:54:17]

http://codereview.chromium.org/4004006/diff/1/6
File src/codegen.cc (right):

http://codereview.chromium.org/4004006/diff/1/6#newcode72
src/codegen.cc:72: } else if (h1->IsHeapNumber() && h2->IsHeapNumber()) {
HeapNumbers should be treated as strings. I think you can create a failing test
case if you do something like:

var x = { 12.2: function() {}, "12.2": 6 };

http://codereview.chromium.org/4004006/diff/1/6#newcode91
src/codegen.cc:91: 
Remove empty line.

http://codereview.chromium.org/4004006/diff/1/6#newcode93
src/codegen.cc:93: Literal*                 literal  = property->key();
We do not align assignments like this. Please just use:

Literal* literal = ...;

http://codereview.chromium.org/4004006/diff/1/6#newcode100
src/codegen.cc:100: // Calculate hash value
We need to respect the split between properties (named) and elements (indexed)
properties here. Only smis that return true from ToArrayIndex should be treated
as elements. The rest of the keys need to be treated as strings (which means
that you need to convert smis and heap numbers that are not array indices to
strings).

You should model this code after runtime.cc CreateObjectLiteralBoilerplate. In
order to avoid repeated conversions you should save the converted key instead of
the literal in the hash table.

http://codereview.chromium.org/4004006/diff/1/9
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/4004006/diff/1/9#newcode5582
src/ia32/codegen-ia32.cc:5582: Load(property->value());
We need to be careful here. The fact that we do not want to store the value does
not mean that we shouldn't compute it. I think you should add a test case where
the right hand side has a side-effect.

var global = 0;
var x = { a: global++, a: 7 };
assertEquals(1, global);

If you omit the evaluation of the value this test will fail.

http://codereview.chromium.org/4004006/diff/1/9#newcode5601
src/ia32/codegen-ia32.cc:5601: Load(property->value());
Same thing here.

http://codereview.chromium.org/4004006/diff/1/10
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/4004006/diff/1/10#newcode39
src/ia32/full-codegen-ia32.cc:39: #include "hashmap.h"
I don't think you need hashmap here?

http://codereview.chromium.org/4004006/diff/1/10#newcode1241
src/ia32/full-codegen-ia32.cc:1241: VisitForStackValue(value);
Need to evaluate this one for side-effects even if you do not emit the store. Be
careful to keep the stack balanced when doing that.

http://codereview.chromium.org/4004006/diff/1/11
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/4004006/diff/1/11#newcode4889
src/x64/codegen-x64.cc:4889: Load(property->value());
Need to evaluate this one.

http://codereview.chromium.org/4004006/diff/1/11#newcode4907
src/x64/codegen-x64.cc:4907: Load(property->value());
Need to evaluate this one.

http://codereview.chromium.org/4004006/diff/1/12
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/4004006/diff/1/12#newcode1200
src/x64/full-codegen-x64.cc:1200: VisitForStackValue(value);
Ditto.

http://codereview.chromium.org/4004006/diff/1/13
File test/mjsunit/object-literal-conversions.js (right):

http://codereview.chromium.org/4004006/diff/1/13#newcode29
test/mjsunit/object-literal-conversions.js:29: // used when overwriting
initializers.
Don't you need computed values in order to really test here?

Expanding this with a few more cases would be good.

http://codereview.chromium.org/4004006/diff/1/14
File test/mjsunit/object-literal-overwrite.js (right):

http://codereview.chromium.org/4004006/diff/1/14#newcode51
test/mjsunit/object-literal-overwrite.js:51: bar: function(b){},
Please indent as above.

http://codereview.chromium.org/4004006/diff/1/14#newcode79
test/mjsunit/object-literal-overwrite.js:79: // Test for the non-full code
generator.
non-full -> classic.

http://codereview.chromium.org/4004006/diff/1/14#newcode84
test/mjsunit/object-literal-overwrite.js:84: do { inner = { j: function(x) {
return x; }, j: 7 } } while(false);
I don't think this has to be that complicated?

http://codereview.chromium.org/4004006/diff/1/14#newcode99
test/mjsunit/object-literal-overwrite.js:99: var bar1 = { x: glob++, x: glob++,
x: glob++, x: 7};
You need to check this for the classic code generator as well.

[Kevin Millikin (Chromium), 2010-10-26 09:14:59]

http://codereview.chromium.org/4004006/diff/1/2
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/4004006/diff/1/2#newcode48
src/arm/codegen-arm.cc:48: #include "codegen.h"
We usually alphabetize includes.  Sometimes it hides implicit dependencies,
sometimes it reveals them, but at least its systematic.

We also have that xxx-inl.h always includes xxx.h, so including codegen-inl.h is
enough to get codegen.h and there's no need for this new include.

http://codereview.chromium.org/4004006/diff/1/3
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/4004006/diff/1/3#newcode356
src/arm/full-codegen-arm.cc:356: }
Our usual style calls for two blank lines between most top-level definitions.

http://codereview.chromium.org/4004006/diff/1/7
File src/codegen.h (right):

http://codereview.chromium.org/4004006/diff/1/7#newcode90
src/codegen.h:90: void CalculateEmitStore(ObjectLiteral *);
We typically avoid these free functions.  In this case, it doesn't have anything
specific to do with this code generator, and properly belongs as a member
function on ObjectLiteral.

http://codereview.chromium.org/4004006/diff/1/8
File src/full-codegen.cc (right):

http://codereview.chromium.org/4004006/diff/1/8#newcode41
src/full-codegen.cc:41: 
This, on the other hand, is too much vertical whitespace!

[Karl Klose, 2010-10-26 11:51:54]

http://codereview.chromium.org/4004006/diff/1/2
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/4004006/diff/1/2#newcode48
src/arm/codegen-arm.cc:48: #include "codegen.h"
On 2010/10/26 09:14:59, Kevin Millikin wrote:
> We usually alphabetize includes.  Sometimes it hides implicit dependencies,
> sometimes it reveals them, but at least its systematic.
> 
> We also have that xxx-inl.h always includes xxx.h, so including codegen-inl.h
is
> enough to get codegen.h and there's no need for this new include.

Done.

http://codereview.chromium.org/4004006/diff/1/3
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/4004006/diff/1/3#newcode356
src/arm/full-codegen-arm.cc:356: }
On 2010/10/26 09:14:59, Kevin Millikin wrote:
> Our usual style calls for two blank lines between most top-level definitions.

Done.

http://codereview.chromium.org/4004006/diff/1/6
File src/codegen.cc (right):

http://codereview.chromium.org/4004006/diff/1/6#newcode72
src/codegen.cc:72: } else if (h1->IsHeapNumber() && h2->IsHeapNumber()) {
On 2010/10/26 08:54:17, Mads Ager wrote:
> HeapNumbers should be treated as strings. I think you can create a failing
test
> case if you do something like:
> 
> var x = { 12.2: function() {}, "12.2": 6 };

Done.

http://codereview.chromium.org/4004006/diff/1/6#newcode91
src/codegen.cc:91: 
On 2010/10/26 08:54:17, Mads Ager wrote:
> Remove empty line.

Done.

http://codereview.chromium.org/4004006/diff/1/6#newcode93
src/codegen.cc:93: Literal*                 literal  = property->key();
On 2010/10/26 08:54:17, Mads Ager wrote:
> We do not align assignments like this. Please just use:
> 
> Literal* literal = ...;

Done.

http://codereview.chromium.org/4004006/diff/1/6#newcode100
src/codegen.cc:100: // Calculate hash value
On 2010/10/26 08:54:17, Mads Ager wrote:
> We need to respect the split between properties (named) and elements (indexed)
> properties here. Only smis that return true from ToArrayIndex should be
treated
> as elements. The rest of the keys need to be treated as strings (which means
> that you need to convert smis and heap numbers that are not array indices to
> strings).
> 
> You should model this code after runtime.cc CreateObjectLiteralBoilerplate. In
> order to avoid repeated conversions you should save the converted key instead
of
> the literal in the hash table.

Done.

http://codereview.chromium.org/4004006/diff/1/7
File src/codegen.h (right):

http://codereview.chromium.org/4004006/diff/1/7#newcode90
src/codegen.h:90: void CalculateEmitStore(ObjectLiteral *);
On 2010/10/26 09:14:59, Kevin Millikin wrote:
> We typically avoid these free functions.  In this case, it doesn't have
anything
> specific to do with this code generator, and properly belongs as a member
> function on ObjectLiteral.

Done, moved to ObjectLiteral.

http://codereview.chromium.org/4004006/diff/1/8
File src/full-codegen.cc (right):

http://codereview.chromium.org/4004006/diff/1/8#newcode41
src/full-codegen.cc:41: 
On 2010/10/26 09:14:59, Kevin Millikin wrote:
> This, on the other hand, is too much vertical whitespace!

Done.

http://codereview.chromium.org/4004006/diff/1/9
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/4004006/diff/1/9#newcode5582
src/ia32/codegen-ia32.cc:5582: Load(property->value());
On 2010/10/26 08:54:17, Mads Ager wrote:
> We need to be careful here. The fact that we do not want to store the value
does
> not mean that we shouldn't compute it. I think you should add a test case
where
> the right hand side has a side-effect.
> 
> var global = 0;
> var x = { a: global++, a: 7 };
> assertEquals(1, global);
> 
> If you omit the evaluation of the value this test will fail.
> 

Done.

http://codereview.chromium.org/4004006/diff/1/9#newcode5601
src/ia32/codegen-ia32.cc:5601: Load(property->value());
On 2010/10/26 08:54:17, Mads Ager wrote:
> Same thing here.

Done.

http://codereview.chromium.org/4004006/diff/1/10
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/4004006/diff/1/10#newcode39
src/ia32/full-codegen-ia32.cc:39: #include "hashmap.h"
On 2010/10/26 08:54:17, Mads Ager wrote:
> I don't think you need hashmap here?

Done.

http://codereview.chromium.org/4004006/diff/1/10#newcode1241
src/ia32/full-codegen-ia32.cc:1241: VisitForStackValue(value);
On 2010/10/26 08:54:17, Mads Ager wrote:
> Need to evaluate this one for side-effects even if you do not emit the store.
Be
> careful to keep the stack balanced when doing that.

Done

http://codereview.chromium.org/4004006/diff/1/11
File src/x64/codegen-x64.cc (right):

http://codereview.chromium.org/4004006/diff/1/11#newcode4889
src/x64/codegen-x64.cc:4889: Load(property->value());
On 2010/10/26 08:54:17, Mads Ager wrote:
> Need to evaluate this one.

Done.

http://codereview.chromium.org/4004006/diff/1/11#newcode4907
src/x64/codegen-x64.cc:4907: Load(property->value());
On 2010/10/26 08:54:17, Mads Ager wrote:
> Need to evaluate this one. 

Done.

http://codereview.chromium.org/4004006/diff/1/12
File src/x64/full-codegen-x64.cc (right):

http://codereview.chromium.org/4004006/diff/1/12#newcode1200
src/x64/full-codegen-x64.cc:1200: VisitForStackValue(value);
On 2010/10/26 08:54:17, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/4004006/diff/1/13
File test/mjsunit/object-literal-conversions.js (right):

http://codereview.chromium.org/4004006/diff/1/13#newcode29
test/mjsunit/object-literal-conversions.js:29: // used when overwriting
initializers.
On 2010/10/26 08:54:17, Mads Ager wrote:
> Don't you need computed values in order to really test here?
> 
> Expanding this with a few more cases would be good.

Done.

http://codereview.chromium.org/4004006/diff/1/14
File test/mjsunit/object-literal-overwrite.js (right):

http://codereview.chromium.org/4004006/diff/1/14#newcode51
test/mjsunit/object-literal-overwrite.js:51: bar: function(b){},
On 2010/10/26 08:54:17, Mads Ager wrote:
> Please indent as above.

Done.

http://codereview.chromium.org/4004006/diff/1/14#newcode79
test/mjsunit/object-literal-overwrite.js:79: // Test for the non-full code
generator.
On 2010/10/26 08:54:17, Mads Ager wrote:
> non-full -> classic.

Done.

http://codereview.chromium.org/4004006/diff/1/14#newcode84
test/mjsunit/object-literal-overwrite.js:84: do { inner = { j: function(x) {
return x; }, j: 7 } } while(false);
On 2010/10/26 08:54:17, Mads Ager wrote:
> I don't think this has to be that complicated?

Done.

http://codereview.chromium.org/4004006/diff/1/14#newcode99
test/mjsunit/object-literal-overwrite.js:99: var bar1 = { x: glob++, x: glob++,
x: glob++, x: 7};
On 2010/10/26 08:54:17, Mads Ager wrote:
> You need to check this for the classic code generator as well.

Done.

[Kevin Millikin (Chromium), 2010-10-26 12:31:18]

A couple more drive-bys, they apply to the other platforms as well.

http://codereview.chromium.org/4004006/diff/18001/19001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/4004006/diff/18001/19001#newcode3640
src/arm/codegen-arm.cc:3640: frame_->Pop();
There is a VirtualFrame::Drop function that can be used to drop multiple
elements and probably emits better code.

You should also use it above to signify that you intend to ignore the return
value.

http://codereview.chromium.org/4004006/diff/18001/19002
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/4004006/diff/18001/19002#newcode39
src/arm/full-codegen-arm.cc:39: #include "codegen.h"
This is probably not needed here either.

[Mads Ager (chromium), 2010-10-26 13:53:54]

This is definitely getting there. One more iteration and I think it's ready. :)

http://codereview.chromium.org/4004006/diff/18001/19002
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/4004006/diff/18001/19002#newcode1212
src/arm/full-codegen-arm.cc:1212: VisitForStackValue(value);
You should always visit and then decide whether to store the value to not loose
side-effects of evaluating the value.

http://codereview.chromium.org/4004006/diff/18001/19003
File src/ast.cc (right):

http://codereview.chromium.org/4004006/diff/18001/19003#newcode184
src/ast.cc:184: bool IsEqual(void* first, void* second) {
I think you should split this into two functions. StringEquals and SmiEquals.
Then you don't need the conditionals here.

http://codereview.chromium.org/4004006/diff/18001/19003#newcode204
src/ast.cc:204: for (int i = this->properties()->length()-1; i >= 0; i--) {
Space before and after '-'. Consider pulling out the length extraction from the
for loop.

http://codereview.chromium.org/4004006/diff/18001/19003#newcode238
src/ast.cc:238: // If the key of a computed property is in the table, mark the
property.
mark the property -> do not emit a store for the property.

http://codereview.chromium.org/4004006/diff/18001/19007
File src/ia32/codegen-ia32.cc (right):

http://codereview.chromium.org/4004006/diff/18001/19007#newcode43
src/ia32/codegen-ia32.cc:43: #include "codegen.h"
Remove?

http://codereview.chromium.org/4004006/diff/18001/19007#newcode5579
src/ia32/codegen-ia32.cc:5579: // Duplicate the object as the IC receiver.
Two spaces inserted by accident I think. :)

http://codereview.chromium.org/4004006/diff/18001/19007#newcode5592
src/ia32/codegen-ia32.cc:5592: frame_->Pop();
Use Drop as suggested by Kevin.

http://codereview.chromium.org/4004006/diff/18001/19007#newcode5606
src/ia32/codegen-ia32.cc:5606: // Ignore the result.
Indent by two more spaces. I would put the comment before the runtime call.

http://codereview.chromium.org/4004006/diff/18001/19007#newcode5608
src/ia32/codegen-ia32.cc:5608: frame_->Pop();
Use Drop.

http://codereview.chromium.org/4004006/diff/18001/19008
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/4004006/diff/18001/19008#newcode39
src/ia32/full-codegen-ia32.cc:39: #include "codegen.h"
Not needed?

[Karl Klose, 2010-10-26 13:55:34]

http://codereview.chromium.org/4004006/diff/18001/19001
File src/arm/codegen-arm.cc (right):

http://codereview.chromium.org/4004006/diff/18001/19001#newcode3640
src/arm/codegen-arm.cc:3640: frame_->Pop();
On 2010/10/26 12:31:18, Kevin Millikin wrote:
> There is a VirtualFrame::Drop function that can be used to drop multiple
> elements and probably emits better code.
> 
> You should also use it above to signify that you intend to ignore the return
> value.

Done.

http://codereview.chromium.org/4004006/diff/18001/19002
File src/arm/full-codegen-arm.cc (right):

http://codereview.chromium.org/4004006/diff/18001/19002#newcode39
src/arm/full-codegen-arm.cc:39: #include "codegen.h"
On 2010/10/26 12:31:18, Kevin Millikin wrote:
> This is probably not needed here either.
That is true. Removed.

[Mads Ager (chromium), 2010-10-27 09:28:53]

LGTM

Could you remove the parenthesis around the BUG= line in the description before
committing and maybe add a newline between the description and the BUG= line?

http://codereview.chromium.org/4004006/diff/50003/64006
File src/ia32/full-codegen-ia32.cc (left):

http://codereview.chromium.org/4004006/diff/50003/64006#oldcode1186
src/ia32/full-codegen-ia32.cc:1186: 
Please add back this line.