-
-
Notifications
You must be signed in to change notification settings - Fork 557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Debugger improvements #2 #810
Conversation
Updated debug information test in EngineTests to reflect changes
Added test for block scoped variable visibility
Cleaned up debugger tests with help from updated debugger statement
Clean-up of DebugHandler Prepared StepMode and CallStack tests for accessor properties
Simplified breakpoint tests
And sorry about the crazy amount of commits, including lots of "no, putting it there was a bad idea" followed almost immediately after by "no it wasn't - back it goes". ;-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work overall! Kudos for code coverage from tests and I only found a few nits. W.r.t. Stuff to do/consider
I think that can be another PR when/if so deemed.
Jint/DebuggerStatementHandling.cs
Outdated
@@ -0,0 +1,9 @@ | |||
namespace Jint | |||
{ | |||
public enum DebuggerStatementHandling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be in Runtime.Debugger namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree - I don't know why I only (briefly) considered the Runtime.Interpreter.Statements namespace. :-)
Jint/DebuggerStatementHandling.cs
Outdated
{ | ||
public enum DebuggerStatementHandling | ||
{ | ||
Ignore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as this is public, can we have short descriptions about modes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added descriptions for the enum members and fixed XML comments in general - replacing <code> with inline <c>. Also renamed the Jint
member to Script
.
Renamed DebuggerStatementHandling.Jint to Script. Added XML comments for choices (and fixed <code> instead of <c>)
Awesome, thanks for tweaking. @sebastienros please take a look. |
Thank you @Jither for this! I hope you find the time and interest to address any issues you might find later on 🚀 |
Thanks for the merge, @lahma and @sebastienros! As for issues later on - almost certainly. If nothing else, I'd like to do some more on the "Stuff to do/consider", but most of it requires diving much further into the Jint code and ES specs - even just the division into more scopes should probably be prepared for modules and other scopes (as far as I recall the Chromium devtools protocol has 9 or 10). In the meantime, I'll probably be focusing on doing the example console UI implementation I mentioned in #674 - see if there are some glaring pain points in the minimal API when trying to do a full-featured debugger, and get something for testing future debugger functionality more easily than adding GUI in WPF XAML. I guess I'll use some minimal implementation of SynchronizationContext/Dispatcher for console apps - which would make it easier for others to adapt the code to WPF/WinForms/whatever, without getting too much of a headache handling Jint executing on a different thread from the UI. |
As mentioned in #674, I've been working on improving the
DebugMode
functionality. This pull request should fix:Globals
/Locals
Globals
/Locals
not being logically separated (Globals
included local variables)Source
in breakpointsGlobals
/Locals
StepMode
, particularly after a breakpointIn addition, it:
debugger
statement (as an optional alternative to the existing VS debugging)Expected major/minor pain points
CallStack
no longer includes argument lists - as mentioned in Documentation on DebugMode #674, to make it work in the general case, something would need to parse and stringify all kinds of expressions that might make up an argument.AllowDebuggerStatement
boolean has been changed to an enumDebuggerStatementHandling
to allow three choices. Not all that happy with the naming of the method or the values (Clr
/Jint
/Ignore
) - maybe, for the latter,Native
,Script
,Ignore
, would be better - or a completely different option construct. The enum probably shouldn't be in the root namespace either.Debugger
namespace at the moment are:AddToDebugCallStack
closer to the call (we now need theICallable
in addition to - or, at the moment, instead of - theExpression
).GlobalEnvironmentRecord
'sGetAllBindingNames
, in order to retrieve the declarative record in addition to the binding object.GetAllBindingNames
is only referenced byDebugHandler
in the currentdev
branch.DebugInformation
has been modified, since locals are no longer mixed intoGlobals
, and the debugCallStack no longer includes argument lists.Stuff to do/consider
There are still some missing features, but those require invasion into the main engine code:
JintCallExpression
notifies theDebugHandler
about the call stack changing, e.g. accessor properties and constructors aren't added to the stack, with all that entails.Globals
/Locals
split is inherited from the originalDebugHandler
code. Maybe the scopes should be the same as e.g. Chromium or Firefox - at least having a separateBlock
scope.CallStack
than just the function name - e.g. location.