LGTM, with comments addressed
http://codereview.chromium.org/914003/diff/12001/13002 File src/factory.h (right): http://codereview.chromium.org/914003/diff/12001/13002#newcode233 src/factory.h:233: static Handle<Code> CopyCode(Handle<Code> code, patched_relocation_info -> reloc_info. http://codereview.chromium.org/914003/diff/12001/13003 File src/heap.cc (right): http://codereview.chromium.org/914003/diff/12001/13003#newcode2284 src/heap.cc:2284: // Copy header and instructions End comment with period (times 3). http://codereview.chromium.org/914003/diff/12001/13004 File src/heap.h (right): http://codereview.chromium.org/914003/diff/12001/13004#newcode616 src/heap.h:616: // Copies code but takes alternative relocation_info. alternative -> an alternative, relocation_info -> relocation info. How about something like "Copy the code and scope inof part of the code object, but insert the provided data as the relocation information."? Rename argument name patched_relocation_info -> reloc_info. http://codereview.chromium.org/914003/diff/12001/13006 File src/liveedit.cc (right): http://codereview.chromium.org/914003/diff/12001/13006#newcode347 src/liveedit.cc:347: Substitute -> Replace to match the naming of the other functions. Why not move the full function definition here to save the separate declaration? http://codereview.chromium.org/914003/diff/12001/13006#newcode411 src/liveedit.cc:411: // Auto-growing buffer for writing relocation info code section. Please extend the comment with why an alternative buffer (to the one in the assembler) is needed here. http://codereview.chromium.org/914003/diff/12001/13006#newcode425 src/liveedit.cc:425: void Write(const RelocInfo* rinfo) { Please add a comment here and in GetResult on how the buffer is filled from the bottom. http://codereview.chromium.org/914003/diff/12001/13006#newcode441 src/liveedit.cc:441: if (buffer_size_ < 2*KB) { Spaces around binary ops. http://codereview.chromium.org/914003/diff/12001/13006#newcode455 src/liveedit.cc:455: // Clear the buffer in debug mode. Use 'int3' instructions to make in3 and opcode 0xCC is not very platform independent. http://codereview.chromium.org/914003/diff/12001/13006#newcode483 src/liveedit.cc:483: // Patch positions in code (changes relocation_info section) and possibly relocation_info -> relocation info http://codereview.chromium.org/914003/diff/12001/13006#newcode512 src/liveedit.cc:512: // Simply patch relocation area of code . at the end of a comment. http://codereview.chromium.org/914003/diff/12001/13006#newcode515 src/liveedit.cc:515: } else { Please rephrase this comment. Regarding the TODO, then shorting the code object in-place probably will not work, as I don't think it is possible to have non-code fillers in the code space, but I am not sure. Shorting objects in-place can be done by adjusting their size and writing fillers (one pointer filler map, two pointer filler map or a byte array of the right size) in the freed-up space after the shorter object. You can try it out, but otherwise I suggest that you add some fillers to the reloc info instead. http://codereview.chromium.org/914003/diff/12001/13006#newcode580 src/liveedit.cc:580: Please add some comments to what the different iterators takes care of. Isn't it possible that the same object will be visited twice? http://codereview.chromium.org/914003/diff/12001/13006#newcode630 src/liveedit.cc:630: Handle<FixedArray> breakPointInfos(debug_info->break_points()); breakPointInfos -> break_point_infos http://codereview.chromium.org/914003/diff/12001/13006#newcode637 src/liveedit.cc:637: int new_position = TranslatePosition(info->source_position()->value(), Indentation. http://codereview.chromium.org/914003/diff/12001/13006#newcode642 src/liveedit.cc:642: // Todo(635): Also patch breakpoint objects in JS. Todo -> TODO http://codereview.chromium.org/914003/diff/12001/13007 File src/runtime.cc (right): http://codereview.chromium.org/914003/diff/12001/13007#newcode8869 src/runtime.cc:8869: } Remove one of the empty lines. http://codereview.chromium.org/914003/diff/12001/13009 File test/mjsunit/debug-liveedit-patch-positions-replace.js (right): http://codereview.chromium.org/914003/diff/12001/13009#newcode31 test/mjsunit/debug-liveedit-patch-positions-replace.js:31: // Scenario: some function is being changed, which causes enclosing function to some function -> a function http://codereview.chromium.org/914003/diff/12001/13009#newcode56 test/mjsunit/debug-liveedit-patch-positions-replace.js:56: // line long enough to change rinfo encoding Uppercase first letter + ending period in comments, and move comment one line up before the "var". http://codereview.chromium.org/914003/diff/12001/13009#newcode69 test/mjsunit/debug-liveedit-patch-positions-replace.js:69: function CallM(changer) { Please be more precise in this comment. It is a call IC you are expecting? http://codereview.chromium.org/914003/diff/12001/13010 File test/mjsunit/debug-liveedit-patch-positions.js (right): http://codereview.chromium.org/914003/diff/12001/13010#newcode92 test/mjsunit/debug-liveedit-patch-positions.js:92: assertEquals(pcArrayBefore.toString(), pcArrayAfter.toString()); Can't you use assertArrayEquals here? http://codereview.chromium.org/914003 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
