http://codereview.chromium.org/115816/diff/1/5 File src/x64/assembler-x64-inl.h (right):
http://codereview.chromium.org/115816/diff/1/5#newcode49 Line 49: *reinterpret_cast<uint32_t*>(pc_) = x; On 2009/05/27 15:05:34, Lasse Reichstein wrote: > You could use > Memory::uint32_at(pc_) = x; > instead. I find it more readable, and it's even a little shorter. Done. http://codereview.chromium.org/115816/diff/1/5#newcode55 Line 55: *reinterpret_cast<uint64_t*>(pc_) = x; On 2009/05/27 15:05:34, Lasse Reichstein wrote: > And you might even add a Memory::uint64_at in 64-bit mode. Done. http://codereview.chromium.org/115816/diff/1/4 File src/x64/assembler-x64.h (right): http://codereview.chromium.org/115816/diff/1/4#newcode418 Line 418: // All 64-bit immediates must have a relocation mode. On 2009/05/27 15:05:34, Lasse Reichstein wrote: > I can see they all have that now. Why must they? I.e., give the reason. > Otherwise someone might just add one without a Mode and remove the comment. For clarity, the user of the assembler should explicitly say what relocation mode applies to each 64-bit immediate in the code. http://codereview.chromium.org/115816/diff/1/4#newcode418 Line 418: // All 64-bit immediates must have a relocation mode. On 2009/05/27 15:25:38, Lasse Reichstein wrote: > And if they don't *have* to have a reloc-mode, make it default to NONE. NONE is not a good default. It is no likelier to be the correct relocation mode than any other. http://codereview.chromium.org/115816/diff/1/4#newcode445 Line 445: void arithmetic_op(byte opcode, Register dst, Register src); On 2009/05/27 15:05:34, Lasse Reichstein wrote: > Could you move these to be private. They seem to be only used internally. Done. http://codereview.chromium.org/115816/diff/1/4#newcode744 Line 744: void emit(Immediate x) { emitl(x.value_); } On 2009/05/27 15:05:34, Lasse Reichstein wrote: > No "inline"? No, everywhere in our codebase, inline functions defined in the class declaration are not explicitly declared inline. http://codereview.chromium.org/115816/diff/1/4#newcode749 Line 749: // REX.W is set. On 2009/05/27 15:05:34, Lasse Reichstein wrote: > The last two lines here seems like implementation detail. > It's ok to have it with the implementation (if it's not obvious from the code), > but here it's unnecessary. It is not implementation detail, but defines the effect of the function. http://codereview.chromium.org/115816 --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
