Some comments, but mostly formatting/style issues. If you address them, it LGTM.
http://codereview.chromium.org/108015/diff/1/3 File src/codegen.cc (right): http://codereview.chromium.org/108015/diff/1/3#newcode409 Line 409: int CodeGenerator::FindInlineRuntimeLUT(Handle<String> name) { There is a comment at the top of codegen.h where we try to list all the codegen files whose implementation is shared between platforms. It should be updated with the new functions. http://codereview.chromium.org/108015/diff/1/3#newcode413 Line 413: InlineRuntimeLUT* entry = kInlineRuntimeLUT + i; I'd write kInlineRuntimeLUT[i] here. http://codereview.chromium.org/108015/diff/1/3#newcode415 Line 415: return i; You could make this return entry, and NULL if not found. http://codereview.chromium.org/108015/diff/1/3#newcode428 Line 428: InlineRuntimeLUT* entry = kInlineRuntimeLUT + idx; kInlineRuntimeLUT[idx], but unnecessary if FindInlineRuntimeLUT returns the pointer. http://codereview.chromium.org/108015/diff/1/3#newcode438 Line 438: CodeGenerator::InlineRuntimeLUT& new_entry, The style guide calls for arguments passed by reference to be const. http://codereview.chromium.org/108015/diff/1/3#newcode442 Line 442: InlineRuntimeLUT* entry = kInlineRuntimeLUT + idx; Same. http://codereview.chromium.org/108015/diff/1/4 File src/ia32/codegen-ia32.cc (right): http://codereview.chromium.org/108015/diff/1/4#newcode4553 Line 4553: __ shr(ebp_as_smi.reg(), times_2); Change times_2 to kSmiTagSize. They happen to be the same (and some existing code relies on it), but times_2 is really part of the instruction encoding and not really an int. http://codereview.chromium.org/108015/diff/1/6 File test/cctest/test-log-ia32.cc (right): http://codereview.chromium.org/108015/diff/1/6#newcode70 Line 70: unsigned int ret_addr, To make the test more nearly 64-bit ready, you could change unsigned int to uintptr_t wherever it is an address cast to an int. If it was only cast for printing, I think the new thing is to use the %p format specifier and pass the pointer, instead of %x and cast. http://codereview.chromium.org/108015/diff/1/6#newcode209 Line 209: NewString("_FastCharCodeAt"), I think this should just be indented 4 from the start of the previous line. http://codereview.chromium.org/108015/diff/1/6#newcode215 Line 215: CHECK(CodeGenerator::PatchInlineRuntimeEntry( Same indentation. http://codereview.chromium.org/108015 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
