LGTM, with comments.
http://codereview.chromium.org/155087/diff/1/2 File src/x64/assembler-x64.cc (right): http://codereview.chromium.org/155087/diff/1/2#newcode1078 Line 1078: emit(Immediate(static_cast<int32_t>(value))); Perhaps a comment that no 8-bit version exists is worthwhile. http://codereview.chromium.org/155087/diff/1/3 File src/x64/disasm-x64.cc (right): http://codereview.chromium.org/155087/diff/1/3#newcode348 Line 348: case 0: All cases of a switch statement are indented 2 spaces, and their bodies a total of 4 spaces. http://codereview.chromium.org/155087/diff/1/3#newcode400 Line 400: int disp = mod == 2 ? *reinterpret_cast<int32_t*>(modrmp + 1) : *(modrmp Add more parentheses, and break at a operator higher in the expression's parse. int disp = (mod == 2) ? *reinterpret .... : *(modrmp + 1); http://codereview.chromium.org/155087/diff/1/3#newcode403 Line 403: return mod == 2 ? 5 : 2; I think parentheses help to spot the condition. http://codereview.chromium.org/155087/diff/1/3#newcode1315 Line 1315: const char* NameConverter::NameOfConstant(byte* addr) const { byte* is Address, isn't it? http://codereview.chromium.org/155087/diff/1/3#newcode1354 Line 1354: DisassemblerX64 d(converter_, false /* do not crash if unimplemented */); Remove the inline comment? Or make it crash if unimplemented? http://codereview.chromium.org/155087/diff/1/3#newcode1358 Line 1358: // The X64 assembler does not currently use constant pools. Make this a TODO, or remove "currently" http://codereview.chromium.org/155087/diff/1/3#newcode1363 Line 1363: /*static*/void Disassembler::Disassemble(FILE* f, byte* begin, byte* end) { Remove the /*static*/. If it were really needed, you could write a comment: // This is a static function. http://codereview.chromium.org/155087 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
