Please add a flag for turning this off (lets have it default to true for
now).
Maybe the flag can just control whether liveedit-delay.js is compiled or not
when loading the debugger.
I like the idea of implementing as much as possible in JavaScript even
though
this requires dragging heap objects through JavaScript.
It is good to have tests which also covers the caese where live edit is not
supported. Maby add some more of these, e.g. changing the number of
arguments to
a function, changing function activated on the stack. It would be good to
know
that this cannot cause strange behawiour or crashes when trying to live edit
stuff that is not supported.
You should also consider adding tests which includes GC's at various places
(e.g. GC after changing the code for an activated function). You can add
more
tests as separate changes.
http://codereview.chromium.org/652027/diff/6001/6002
File src/SConscript (right):
http://codereview.chromium.org/652027/diff/6001/6002#newcode252
src/SConscript:252: liveedit-delay.js
This needs to be added to tools/gyp/v8.gyp and tools/visual_studio/...
as well (grep for debug-delay in tools and its subdirectories).
http://codereview.chromium.org/652027/diff/6001/6005
File src/debug-delay.js (right):
http://codereview.chromium.org/652027/diff/6001/6005#newcode1959
src/debug-delay.js:1959:
DebugCommandProcessor.prototype.changeLiveRequest_ = function(request,
response) {
How about adding a check for existence of function
Debug.LiveEditChangeScript? Then the live edit support can be turned off
by just not loading liveedit-delay.js when the debugger is loaded.
http://codereview.chromium.org/652027/diff/6001/6005#newcode1986
src/debug-delay.js:1986: Debug.LiveEditChangeScript(the_script,
change_pos, change_len, new_string,
Intentation of change_log.
http://codereview.chromium.org/652027/diff/6001/6007
File src/debug.h (right):
http://codereview.chromium.org/652027/diff/6001/6007#newcode608
src/debug.h:608: enum AfterCompileFlag {
Please rename Flag -> Flags and FLAG -> FLAGS.
http://codereview.chromium.org/652027/diff/6001/6008
File src/liveedit-delay.js (right):
http://codereview.chromium.org/652027/diff/6001/6008#newcode35
src/liveedit-delay.js:35: // As a result an only one function will have
its Code changed. All nested
First sentence here is broken.
http://codereview.chromium.org/652027/diff/6001/6008#newcode37
src/liveedit-delay.js:37: // and linked to an old version of the script.
(Generally speaking, during the
and linked to an old version of the script -> and keep the original
script.
http://codereview.chromium.org/652027/diff/6001/6008#newcode39
src/liveedit-delay.js:39: // nested functions is introduced.) All other
functions just have
is -> are
http://codereview.chromium.org/652027/diff/6001/6008#newcode66
src/liveedit-delay.js:66: // Find all SharedFunctionInfo's that are
linked to this script.
that are linked to this script -> compiled from this script
http://codereview.chromium.org/652027/diff/6001/6008#newcode67
src/liveedit-delay.js:67: var shared_infos_raw =
%LiveEditFindScriptFunctionInfos(script);
We normally use either shared or shared_function_info for variables
holding ShardsFunctionInfo objects.
http://codereview.chromium.org/652027/diff/6001/6008#newcode70
src/liveedit-delay.js:70: // apply to indexes inside array that stores
these objects.
Can I suggest that you move all the inner functions to the top so they
are not interleaved with the running code for this function?
http://codereview.chromium.org/652027/diff/6001/6008#newcode160
src/liveedit-delay.js:160: // TODO(peter.rybin): fix exception chaining.
Either remove TODO or create an issue and refer to that.
http://codereview.chromium.org/652027/diff/6001/6008#newcode166
src/liveedit-delay.js:166: function ChooseChangedFunction(compile_info,
offset, len) {
Choose -> Find
http://codereview.chromium.org/652027/diff/6001/6008#newcode169
src/liveedit-delay.js:169: // the one, that is later in this list.
Remove the
http://codereview.chromium.org/652027/diff/6001/6008#newcode175
src/liveedit-delay.js:175: // Now we are standing at the last function
that begins before the change
Remove standing.
http://codereview.chromium.org/652027/diff/6001/6008#newcode185
src/liveedit-delay.js:185: // A list of function that must be dropped
from the stack before changes are
function -> functions
http://codereview.chromium.org/652027/diff/6001/6008#newcode187
src/liveedit-delay.js:187: var functions_remove_from_stack = new
Array();
I think the name functions_remove_from_stack is somewhat confusing. As
far as I can see it contains the shared functions info which have new
code.
Currently it is not used, so maybe just remove it for now.
http://codereview.chromium.org/652027/diff/6001/6008#newcode193
src/liveedit-delay.js:193: // same indexes.
Do you want to keep this debugging code?
http://codereview.chromium.org/652027/diff/6001/6008#newcode204
src/liveedit-delay.js:204: // Check that function has the same number of
parameters (they may exist
they -> there
http://codereview.chromium.org/652027/diff/6001/6008#newcode241
src/liveedit-delay.js:241: // TODO(peter.rybin): Check frames on stack
against
No TODO's with username.
http://codereview.chromium.org/652027/diff/6001/6008#newcode244
src/liveedit-delay.js:244: // Committing all changes.
Remove empty line after comment.
http://codereview.chromium.org/652027/diff/6001/6008#newcode287
src/liveedit-delay.js:287:
PatchCode(new_compile_info[function_being_patched],
Indentation.
http://codereview.chromium.org/652027/diff/6001/6008#newcode311
src/liveedit-delay.js:311: // TODO(peter.rybin): explain what is
happening.
No ToDO's with username.
http://codereview.chromium.org/652027/diff/6001/6009
File src/liveedit.cc (right):
http://codereview.chromium.org/652027/diff/6001/6009#newcode47
src/liveedit.cc:47: // TODO(peter.rybin): support extensions.
No TODO with username.
http://codereview.chromium.org/652027/diff/6001/6009#newcode51
src/liveedit.cc:51:
Code in comments.
http://codereview.chromium.org/652027/diff/6001/6009#newcode54
src/liveedit.cc:54:
Nested #ifdef equal to outer #ifdef.
http://codereview.chromium.org/652027/diff/6001/6009#newcode55
src/liveedit.cc:55: #ifdef ENABLE_DEBUGGER_SUPPORT
I don't understand this commetn.
http://codereview.chromium.org/652027/diff/6001/6009#newcode58
src/liveedit.cc:58: // Debugger::OnBeforeCompile(script);
Code in comment.
http://codereview.chromium.org/652027/diff/6001/6009#newcode83
src/liveedit.cc:83:
Please remove this bool, and just change the comment below to something
like "No profiler support for live edit code"
http://codereview.chromium.org/652027/diff/6001/6009#newcode90
src/liveedit.cc:90:
Code in comments
http://codereview.chromium.org/652027/diff/6001/6009#newcode254
src/liveedit.cc:254: }
Empty line between functions.
http://codereview.chromium.org/652027/diff/6001/6009#newcode302
src/liveedit.cc:302: // TODO(peter.rybin): next time try bubble sort.
No TODO's with username. Here and other places.
http://codereview.chromium.org/652027/diff/6001/6009#newcode323
src/liveedit.cc:323: int test_len =
Smi::cast(scope_info_list->length())->value();
Remove debugging code.
http://codereview.chromium.org/652027/diff/6001/6009#newcode338
src/liveedit.cc:338: }
Empty line between functions.
http://codereview.chromium.org/652027/diff/6001/6009#newcode350
src/liveedit.cc:350: JSArray* LiveEdit::GatherCompileInfo(Handle<Script>
script,
Indentation.
http://codereview.chromium.org/652027/diff/6001/6009#newcode364
src/liveedit.cc:364:
Two empty lines here.
http://codereview.chromium.org/652027/diff/6001/6009#newcode403
src/liveedit.cc:403:
Two empty lines here. And generally two empty lines between functions on
top level.
http://codereview.chromium.org/652027/diff/6001/6009#newcode404
src/liveedit.cc:404: /**
Please use // style comments.
http://codereview.chromium.org/652027/diff/6001/6009#newcode451
src/liveedit.cc:451: #endif // ENABLE_DEBUGGER_SUPPORT
You can remove this #endif and the #ifdef just below, and move the
comment on an empty implementation to after the #else.
http://codereview.chromium.org/652027/diff/6001/6009#newcode483
src/liveedit.cc:483: #else
Please add comment // ENABLE_DEBUGGER_SUPPORT after #else as well
http://codereview.chromium.org/652027/diff/6001/6011
File src/runtime.cc (right):
http://codereview.chromium.org/652027/diff/6001/6011#newcode8219
src/runtime.cc:8219: static int FindFunctionInfoForScript(Script*
script,
FindFunctionInfoForScript -> FindSharedFunctionInfosForScript (please
keep the actual names to avoid confusion).
http://codereview.chromium.org/652027/diff/6001/6011#newcode8235
src/runtime.cc:8235: if (counter < buffer_size) {
Please remove argument UPDATE_WRITE_BARRIER. the set function with two
arguments take care of this.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8243
src/runtime.cc:8243: /**
Please use // style comments.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8248
src/runtime.cc:8248: static Object*
Runtime_LiveEditFindScriptFunctionInfos(Arguments args) {
Runtime_LiveEditFindScriptFunctionInfos ->
Runtime_LiveEditFindSharedFunctionInfosForScript.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8251
src/runtime.cc:8251: CONVERT_CHECKED(JSValue, script, args[0]);
Maybe script -> script_value here and script_handle -> script below for
consistency.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8299
src/runtime.cc:8299: * Changes the text of the script to a new
new_source and creates a new
Changes the text of the script to a new new_source -> Changes the souce
of a script
http://codereview.chromium.org/652027/diff/6001/6011#newcode8300
src/runtime.cc:8300: * instance of script representing the old version
of the script source.
Delete "instance of"
http://codereview.chromium.org/652027/diff/6001/6011#newcode8347
src/runtime.cc:8347: * Connects SharedFunctionInfo to another script
(representing the old
Please remove the part in ()'s as it indicate something about the caller
which is not checked.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8353
src/runtime.cc:8353: CONVERT_ARG_CHECKED(JSArray, shared_info_array, 0);
script -> script_value, script_handle ->script and CONVERT_CHECKED ->
CONVERT_ARG_CHECKED to avoid mixing pointers and handles.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8363
src/runtime.cc:8363: * Updates positions of a function (first parameter)
according to script
a function -> a number shared function info
http://codereview.chromium.org/652027/diff/6001/6011#newcode8364
src/runtime.cc:8364: * text change. Text change is described in second
parameter as array of
text -> source
http://codereview.chromium.org/652027/diff/6001/6011#newcode8371
src/runtime.cc:8371: CONVERT_ARG_CHECKED(JSArray, shared_info_array, 0);
Use either shared_array or shared_function_info_array instead of
shared_info_array.
http://codereview.chromium.org/652027
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
