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, On 2010/03/15 08:41:16, Søren Gjesse wrote:
patched_relocation_info -> reloc_info.
Done. 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 On 2010/03/15 08:41:16, Søren Gjesse wrote:
End comment with period (times 3).
Done*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. On 2010/03/15 08:41:16, Søren Gjesse wrote:
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.
Cool. Done. 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: On 2010/03/15 08:41:16, Søren Gjesse wrote:
Substitute -> Replace to match the naming of the other functions. Why
not move
the full function definition here to save the separate declaration?
Done. http://codereview.chromium.org/914003/diff/12001/13006#newcode411 src/liveedit.cc:411: // Auto-growing buffer for writing relocation info code section. On 2010/03/15 08:41:16, Søren Gjesse wrote:
Please extend the comment with why an alternative buffer (to the one
in the
assembler) is needed here.
Done. http://codereview.chromium.org/914003/diff/12001/13006#newcode425 src/liveedit.cc:425: void Write(const RelocInfo* rinfo) { On 2010/03/15 08:41:16, Søren Gjesse wrote:
Please add a comment here and in GetResult on how the buffer is filled
from the
bottom.
Done. http://codereview.chromium.org/914003/diff/12001/13006#newcode441 src/liveedit.cc:441: if (buffer_size_ < 2*KB) { On 2010/03/15 08:41:16, Søren Gjesse wrote:
Spaces around binary ops.
Done. http://codereview.chromium.org/914003/diff/12001/13006#newcode455 src/liveedit.cc:455: // Clear the buffer in debug mode. Use 'int3' instructions to make On 2010/03/15 08:41:16, Søren Gjesse wrote:
in3 and opcode 0xCC is not very platform independent.
I simply removed it. Do we anything better, e.g. any API for platform-independent memory filler? http://codereview.chromium.org/914003/diff/12001/13006#newcode483 src/liveedit.cc:483: // Patch positions in code (changes relocation_info section) and possibly On 2010/03/15 08:41:16, Søren Gjesse wrote:
relocation_info -> relocation info
Done. http://codereview.chromium.org/914003/diff/12001/13006#newcode512 src/liveedit.cc:512: // Simply patch relocation area of code On 2010/03/15 08:41:16, Søren Gjesse wrote:
. at the end of a comment.
Done. http://codereview.chromium.org/914003/diff/12001/13006#newcode515 src/liveedit.cc:515: } else { On 2010/03/15 08:41:16, Søren Gjesse wrote:
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.
Done Speaking about TODO I think I can safely drop it, because it would make the code too complicated for now real benefit. http://codereview.chromium.org/914003/diff/12001/13006#newcode580 src/liveedit.cc:580: On 2010/03/15 08:41:16, Søren Gjesse wrote:
Please add some comments to what the different iterators takes care
of. Isn't it
possible that the same object will be visited twice?
Done. I'm not sure. However, I need to iterate over all pointers. I thought that there are roots and there are pointers inside objects and that is roughly all pointers in the system and these 2 sets do not overlap. Do you think it is also worth making into a comment? http://codereview.chromium.org/914003/diff/12001/13006#newcode630 src/liveedit.cc:630: Handle<FixedArray> breakPointInfos(debug_info->break_points()); On 2010/03/15 08:41:16, Søren Gjesse wrote:
breakPointInfos -> break_point_infos
Done. http://codereview.chromium.org/914003/diff/12001/13006#newcode637 src/liveedit.cc:637: int new_position = TranslatePosition(info->source_position()->value(), On 2010/03/15 08:41:16, Søren Gjesse wrote:
Indentation.
Done. http://codereview.chromium.org/914003/diff/12001/13006#newcode642 src/liveedit.cc:642: // Todo(635): Also patch breakpoint objects in JS. On 2010/03/15 08:41:16, Søren Gjesse wrote:
Todo -> TODO
Done. 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: } On 2010/03/15 08:41:16, Søren Gjesse wrote:
Remove one of the empty lines.
Done. 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 On 2010/03/15 08:41:16, Søren Gjesse wrote:
some function -> a function
Done. 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 On 2010/03/15 08:41:16, Søren Gjesse wrote:
Uppercase first letter + ending period in comments, and move comment
one line up
before the "var".
Done. http://codereview.chromium.org/914003/diff/12001/13009#newcode69 test/mjsunit/debug-liveedit-patch-positions-replace.js:69: function CallM(changer) { On 2010/03/15 08:41:16, Søren Gjesse wrote:
Please be more precise in this comment. It is a call IC you are
expecting? Done. 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()); On 2010/03/15 08:41:16, Søren Gjesse wrote:
Can't you use assertArrayEquals here?
Thanks. Done. http://codereview.chromium.org/914003 -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
