Hi Soren
I think I fixed on every comment of yours.
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 did so. The flag is called enable_liveedit.
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.
That's in my TODO.
Peter
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
On 2010/03/04 09:45:25, Søren Gjesse wrote:
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).
Done.
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) {
On 2010/03/04 09:45:25, Søren Gjesse wrote:
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.
Great idea.
Done
http://codereview.chromium.org/652027/diff/6001/6005#newcode1986
src/debug-delay.js:1986: Debug.LiveEditChangeScript(the_script,
change_pos, change_len, new_string,
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Intentation of change_log.
Done.
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 {
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Please rename Flag -> Flags and FLAG -> FLAGS.
Done.
I also changed corresponding condition to bitwise.
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
On 2010/03/04 09:45:25, Søren Gjesse wrote:
First sentence here is broken.
Done.
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
On 2010/03/04 09:45:25, Søren Gjesse wrote:
and linked to an old version of the script -> and keep the original
script.
It actually works differently. I rephrased accordingly.
http://codereview.chromium.org/652027/diff/6001/6008#newcode39
src/liveedit-delay.js:39: // nested functions is introduced.) All other
functions just have
On 2010/03/04 09:45:25, Søren Gjesse wrote:
is -> are
Done.
http://codereview.chromium.org/652027/diff/6001/6008#newcode66
src/liveedit-delay.js:66: // Find all SharedFunctionInfo's that are
linked to this script.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
that are linked to this script -> compiled from this script
This may be less accurate (because of manual relinking to old scripts),
but probably it's fine.
Done
http://codereview.chromium.org/652027/diff/6001/6008#newcode67
src/liveedit-delay.js:67: var shared_infos_raw =
%LiveEditFindScriptFunctionInfos(script);
On 2010/03/04 09:45:25, Søren Gjesse wrote:
We normally use either shared or shared_function_info for variables
holding
ShardsFunctionInfo objects.
Done.
http://codereview.chromium.org/652027/diff/6001/6008#newcode70
src/liveedit-delay.js:70: // apply to indexes inside array that stores
these objects.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
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?
Done.
However, most of them are closures that use local variables now before
they are declared with "var". Probably we'll have to deal with it
somehow.
http://codereview.chromium.org/652027/diff/6001/6008#newcode160
src/liveedit-delay.js:160: // TODO(peter.rybin): fix exception chaining.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Either remove TODO or create an issue and refer to that.
Do you mean V8 team does not use TODO in sources without issue
registered?
What would you do if the issue is too small to be submitted on server
like in many cases here?
Can I used parameterless TODO instead?
Done
http://codereview.chromium.org/652027/diff/6001/6008#newcode166
src/liveedit-delay.js:166: function ChooseChangedFunction(compile_info,
offset, len) {
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Choose -> Find
Done.
http://codereview.chromium.org/652027/diff/6001/6008#newcode169
src/liveedit-delay.js:169: // the one, that is later in this list.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Remove the
Done
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
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Remove standing.
Done.
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
On 2010/03/04 09:45:25, Søren Gjesse wrote:
function -> functions
Done.
http://codereview.chromium.org/652027/diff/6001/6008#newcode187
src/liveedit-delay.js:187: var functions_remove_from_stack = new
Array();
On 2010/03/04 09:45:25, Søren Gjesse wrote:
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.
Done.
http://codereview.chromium.org/652027/diff/6001/6008#newcode193
src/liveedit-delay.js:193: // same indexes.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Do you want to keep this debugging code?
I'd rather keep it. This seems to be a good assert which is valuable for
a experimental code.
I hope the name suggests that is should be dropped or refactored in the
future.
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
On 2010/03/04 09:45:25, Søren Gjesse wrote:
they -> there
Done.
http://codereview.chromium.org/652027/diff/6001/6008#newcode241
src/liveedit-delay.js:241: // TODO(peter.rybin): Check frames on stack
against
On 2010/03/04 09:45:25, Søren Gjesse wrote:
No TODO's with username.
I removed username. Can I do like this?
Done.
http://codereview.chromium.org/652027/diff/6001/6008#newcode244
src/liveedit-delay.js:244: // Committing all changes.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Remove empty line after comment.
I wanted this to be a standalone comment, that splits the function into
2 parts (preparing and committing).
Done
http://codereview.chromium.org/652027/diff/6001/6008#newcode287
src/liveedit-delay.js:287:
PatchCode(new_compile_info[function_being_patched],
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Indentation.
Done.
http://codereview.chromium.org/652027/diff/6001/6008#newcode311
src/liveedit-delay.js:311: // TODO(peter.rybin): explain what is
happening.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
No ToDO's with username.
Done.
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.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
No TODO with username.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode51
src/liveedit.cc:51:
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Code in comments.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode54
src/liveedit.cc:54:
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Nested #ifdef equal to outer #ifdef.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode55
src/liveedit.cc:55: #ifdef ENABLE_DEBUGGER_SUPPORT
On 2010/03/04 09:45:25, Søren Gjesse wrote:
I don't understand this commetn.
Removed
http://codereview.chromium.org/652027/diff/6001/6009#newcode58
src/liveedit.cc:58: // Debugger::OnBeforeCompile(script);
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Code in comment.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode83
src/liveedit.cc:83:
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Please remove this bool, and just change the comment below to
something like "No
profiler support for live edit code"
Well, I removed all the code commented around. Now this only comment
remained. I guess it's obvious there is no profiler support here because
you can't see profiler support here. :)
Removed the comment.
These comments were needed because I copy-pasted it from compiler code
and wasn't sure which part I may safely drop.
http://codereview.chromium.org/652027/diff/6001/6009#newcode90
src/liveedit.cc:90:
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Code in comments
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode254
src/liveedit.cc:254: }
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Empty line between functions.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode302
src/liveedit.cc:302: // TODO(peter.rybin): next time try bubble sort.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
No TODO's with username. Here and other places.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode323
src/liveedit.cc:323: int test_len =
Smi::cast(scope_info_list->length())->value();
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Remove debugging code.
My bad.
Done
http://codereview.chromium.org/652027/diff/6001/6009#newcode338
src/liveedit.cc:338: }
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Empty line between functions.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode350
src/liveedit.cc:350: JSArray* LiveEdit::GatherCompileInfo(Handle<Script>
script,
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Indentation.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode364
src/liveedit.cc:364:
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Two empty lines here.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode403
src/liveedit.cc:403:
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Two empty lines here. And generally two empty lines between functions
on top
level.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode404
src/liveedit.cc:404: /**
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Please use // style comments.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode451
src/liveedit.cc:451: #endif // ENABLE_DEBUGGER_SUPPORT
On 2010/03/04 09:45:25, Søren Gjesse wrote:
You can remove this #endif and the #ifdef just below, and move the
comment on an
empty implementation to after the #else.
Done.
http://codereview.chromium.org/652027/diff/6001/6009#newcode483
src/liveedit.cc:483: #else
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Please add comment // ENABLE_DEBUGGER_SUPPORT after #else as well
Done.
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,
On 2010/03/04 09:45:25, Søren Gjesse wrote:
FindFunctionInfoForScript -> FindSharedFunctionInfosForScript (please
keep the
actual names to avoid confusion).
Done.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8235
src/runtime.cc:8235: if (counter < buffer_size) {
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Please remove argument UPDATE_WRITE_BARRIER. the set function with two
arguments
take care of this.
Done.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8243
src/runtime.cc:8243: /**
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Please use // style comments.
Done.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8248
src/runtime.cc:8248: static Object*
Runtime_LiveEditFindScriptFunctionInfos(Arguments args) {
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Runtime_LiveEditFindScriptFunctionInfos ->
Runtime_LiveEditFindSharedFunctionInfosForScript.
Done.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8251
src/runtime.cc:8251: CONVERT_CHECKED(JSValue, script, args[0]);
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Maybe script -> script_value here and script_handle -> script below
for
consistency.
Done.
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
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Changes the text of the script to a new new_source -> Changes the
souce of a
script
Done.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8300
src/runtime.cc:8300: * instance of script representing the old version
of the script source.
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Delete "instance of"
Done.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8347
src/runtime.cc:8347: * Connects SharedFunctionInfo to another script
(representing the old
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Please remove the part in ()'s as it indicate something about the
caller which
is not checked.
Done.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8353
src/runtime.cc:8353: CONVERT_ARG_CHECKED(JSArray, shared_info_array, 0);
On 2010/03/04 09:45:25, Søren Gjesse wrote:
script -> script_value, script_handle ->script and CONVERT_CHECKED ->
CONVERT_ARG_CHECKED to avoid mixing pointers and handles.
Done.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8363
src/runtime.cc:8363: * Updates positions of a function (first parameter)
according to script
On 2010/03/04 09:45:25, Søren Gjesse wrote:
a function -> a number shared function info
Didn't get what "number" stands for. Ignored it for now.
Do you confirm the wording is right?
Done
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
On 2010/03/04 09:45:25, Søren Gjesse wrote:
text -> source
Done.
http://codereview.chromium.org/652027/diff/6001/6011#newcode8371
src/runtime.cc:8371: CONVERT_ARG_CHECKED(JSArray, shared_info_array, 0);
On 2010/03/04 09:45:25, Søren Gjesse wrote:
Use either shared_array or shared_function_info_array instead of
shared_info_array.
Done.
http://codereview.chromium.org/652027
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
