A couple of comments. Also, you should modify x64 and arm as well.


http://codereview.chromium.org/6240012/diff/6001/src/handles.cc
File src/handles.cc (left):

http://codereview.chromium.org/6240012/diff/6001/src/handles.cc#oldcode871
src/handles.cc:871: OptimizedObjectForAddingMultipleProperties::
Is there a change to the header file missing here. If you remove the
implementation you should remove the declaration as well.

Getting rid of the longest class name in the system. Nice. ;)

http://codereview.chromium.org/6240012/diff/6001/src/ia32/full-codegen-ia32.cc
File src/ia32/full-codegen-ia32.cc (right):

http://codereview.chromium.org/6240012/diff/6001/src/ia32/full-codegen-ia32.cc#newcode1405
src/ia32/full-codegen-ia32.cc:1405: if (expr->fast_elements() &&
expr->has_function()) {
I'm not sure I understand why fast elements is part of this condition?
The function part is about the properties which should have nothing to
do with the elements? Shouldn't this just check expr->has_function and
not care about the elements?

http://codereview.chromium.org/6240012/diff/6001/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/6240012/diff/6001/src/ia32/lithium-codegen-ia32.cc#newcode3362
src/ia32/lithium-codegen-ia32.cc:3362: }
Shouldn't this contain the call to go back to fast elements as well?

http://codereview.chromium.org/6240012/diff/6001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/6240012/diff/6001/src/runtime.cc#newcode307
src/runtime.cc:307: // slow properties mode for now and we don't go in
the map cache.
Please expand the comment to say that we cannot share the maps in any
case because the constant functions will rarely (never?) be the same.

http://codereview.chromium.org/6240012/

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to