PTAL.
http://codereview.chromium.org/8241003/diff/1008/src/code-stubs.h File src/code-stubs.h (right): http://codereview.chromium.org/8241003/diff/1008/src/code-stubs.h#newcode1041 src/code-stubs.h:1041: private: On 2011/10/13 09:15:59, Jakob wrote:
nit: classes, then methods, then data members. (see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declaration_Order#Declaration_Order) Done. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc File src/ia32/code-stubs-ia32.cc (right): http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7065 src/ia32/code-stubs-ia32.cc:7065: // edi: source FixedArray On 2011/10/13 09:15:59, Jakob wrote:
You can omit all comments for registers whose contents have not
changed since
the last comment. Up to you.
Done. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7115 src/ia32/code-stubs-ia32.cc:7115: // Restore necessary values do store. On 2011/10/13 09:15:59, Jakob wrote:
I don't understand this comment.
Removed and replaced by comments below. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7132 src/ia32/code-stubs-ia32.cc:7132: // Restore esp and esi before jumping into runtime. On 2011/10/13 09:15:59, Jakob wrote:
Slightly outdated. s/esp and esi/registers/?
Done. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7187 src/ia32/code-stubs-ia32.cc:7187: __ mov(esi, FieldOperand(edi, ebx, times_4, FixedDoubleArray::kHeaderSize)); On 2011/10/13 09:15:59, Jakob wrote:
You could check if SSE2 is available and if yes, do a single 64bit
move using an
xmm register, falling back to two 32bit moves via esi otherwise.
Done. http://codereview.chromium.org/8241003/diff/1008/src/ia32/code-stubs-ia32.cc#newcode7223 src/ia32/code-stubs-ia32.cc:7223: __ mov(ebx, eax); On 2011/10/13 09:15:59, Jakob wrote:
This move is unnecessary. EAX is popped next anyway, so it's OK for it
to be
clobbered.
Done. http://codereview.chromium.org/8241003/diff/1008/src/ic.cc File src/ic.cc (right): http://codereview.chromium.org/8241003/diff/1008/src/ic.cc#newcode1784 src/ic.cc:1784: On 2011/10/13 09:15:59, Jakob wrote:
nit: remove this empty line
Done. http://codereview.chromium.org/8241003/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
