Mostly LGTM, I'll land this for you if you address my comments. Most of the
comments on test-assembler-ia32.cc apply to x64 as well.
https://codereview.chromium.org/18300003/diff/1/src/platform-win32.cc
File src/platform-win32.cc (right):
https://codereview.chromium.org/18300003/diff/1/src/platform-win32.cc#newcode1486
src/platform-win32.cc:1486: #endif
Let's use an #else here, and keep the #endif at the end of the
functions. Some compilers don't like unreachable code after
unconditional early returns.
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-assembler-ia32.cc
File test/cctest/test-assembler-ia32.cc (right):
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-assembler-ia32.cc#newcode490
test/cctest/test-assembler-ia32.cc:490: if
(CpuFeatures::IsSupported(SSE2)) {
I think you should just ASSERT(CpuFeatures::IsSupported(SSE2)) at the
beginning of this function to make it clear that it must not be called
when SSE2 is not supported.
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-assembler-ia32.cc#newcode493
test/cctest/test-assembler-ia32.cc:493: // Remove return address from
the stack for fix stack frame alignment
nit: All comments should end with appropriate punctuation, e.g. a full
stop. (This applies to pretty much all comments in this patch.)
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-assembler-ia32.cc#newcode498
test/cctest/test-assembler-ia32.cc:498: __
push(Immediate(vec->Get(i)->Int32Value()));
nit: {} please
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-assembler-ia32.cc#newcode525
test/cctest/test-assembler-ia32.cc:525: Code::cast(code)->Print();
debugging leftover?
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-assembler-ia32.cc#newcode528
test/cctest/test-assembler-ia32.cc:528: ::printf("f() = %d\n", res);
debugging leftover?
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-assembler-ia32.cc#newcode545
test/cctest/test-assembler-ia32.cc:545: "function foo(vec) {"
nit: indentation (4 spaces when you break inside a call's arguments
list)
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-assembler-ia32.cc#newcode546
test/cctest/test-assembler-ia32.cc:546: " return do_sse2(vec);"
nit: indentation (2 spaces inside function body, i.e.: " return ...)
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-assembler-ia32.cc#newcode551
test/cctest/test-assembler-ia32.cc:551:
v8::Local<v8::Function>::Cast(global_object->Get(v8_str("foo")));
nit: indentation (4 spaces when you break after '=')
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-assembler-ia32.cc#newcode556
test/cctest/test-assembler-ia32.cc:556: v8_vec->Set(i, v8_num(vec[i]));
nit: {} please
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-assembler-ia32.cc#newcode568
test/cctest/test-assembler-ia32.cc:568:
nit: two empty lines is enough
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-platform.cc
File test/cctest/test-platform.cc (right):
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-platform.cc#newcode84
test/cctest/test-platform.cc:84: "function foo() {"
nit: indentation
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-platform.cc#newcode85
test/cctest/test-platform.cc:85: " return get_stack_pointer();"
nit: indentation of 'return'
https://codereview.chromium.org/18300003/diff/1/test/cctest/test-platform.cc#newcode90
test/cctest/test-platform.cc:90:
v8::Local<v8::Function>::Cast(global_object->Get(v8_str("foo")));
nit: indentation
https://codereview.chromium.org/18300003/
--
--
v8-dev mailing list
v8-dev@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to v8-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
