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. http://codereview.chromium.org/4004006/show -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
