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