Some minifies use `,` instead of `;` in many situations, and frankly the `,` operator is basically an inline-statement, so we should treat each comma sub-expression as a statement for pausing (including breakpoints).
Created attachment 395438 [details] Patch
Comment on attachment 395438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395438&action=review > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3045 > + for (; node && node->next(); node = node->next()) { I wonder if the `node` check is actually necessary here. Given we always fall out of this loop assuming `node` is valid. > Source/WebInspectorUI/UserInterface/Workers/Formatter/JSFormatter.js:217 > + switch (node.type) { > + case "BlockStatement": > + case "ClassDeclaration": > + case "DoWhileStatement": > + case "ForInStatement": > + case "ForOfStatement": > + case "ForStatement": > + case "FunctionDeclaration": > + case "IfStatement": > + case "SwitchStatement": > + case "TryStatement": > + case "WhileStatement": > + case "WithStatement": > + return true; Previously this was sorted by frequency of statement, to "optimize" the fewest number of checks. I'm not sure it mattered. Switch is probably fine! > LayoutTests/inspector/debugger/stepping/stepOver-expected.txt:222 > + -> 40 let a = 1|, > + 41 b = 2, > + 42 c = 3; Wouldn't we rather pause before "b"? This seems poor. > LayoutTests/inspector/debugger/stepping/stepOver-expected.txt:247 > + -> 43 testAlert("comma log 1"), |testAlert("comma log 2"), testAlert("comma log 3"); This looks correct tho! > LayoutTests/inspector/formatting/resources/javascript-tests/sample-jquery-expected.js:4165 > - }), e.jQuery = e.$ = b, "function" == typeof define && define.amd && define.amd.jQuery && define("jquery", [], function() { > + }), > + e.jQuery = e.$ = b, Awesome improvement in general!
Comment on attachment 395438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395438&action=review >> Source/WebInspectorUI/UserInterface/Workers/Formatter/JSFormatter.js:217 >> + return true; > > Previously this was sorted by frequency of statement, to "optimize" the fewest number of checks. I'm not sure it mattered. Switch is probably fine! I was wondering how this was sorted! In that case, I would've probably expected DoWhileStatement to be lower down :P We're running this on a `Worker`, so this is probably fine :) >> LayoutTests/inspector/debugger/stepping/stepOver-expected.txt:222 >> + 42 c = 3; > > Wouldn't we rather pause before "b"? This seems poor. I agree, but I'm not really sure what to do about this, especially since it's coming from the same `CommaNode` 🤔 >> LayoutTests/inspector/formatting/resources/javascript-tests/sample-jquery-expected.js:4165 >> + e.jQuery = e.$ = b, > > Awesome improvement in general! Yeah! I think this looks _way_ better :)
Created attachment 395495 [details] Patch Turns out, the only reason I needed to have `m_lastExecutedPosition` was because I was unnecessarily `emitDebugHook` before the first `CommaNode`, which really should already be handled by the container `StatementNode`.In addition to considering each `CommaNode` as a pause location, we should also consider them as a possible breakpoint location.
Comment on attachment 395495 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395495&action=review Do we have a test for something like: for(let x = 1, y = 2; ...; ...) {} I want to make sure we don't pretty print that poorly, and that we'd pause on `y=2` naturally. And maybe just sanity check this as well (though I don't expect it to be any different): let x; if (x = 1, x) {} > Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3045 > + for (; node && node->next(); node = node->next()) { I still think we can and should drop the `node` at the start of the condition. If it was ever nullptr we'd crash at line 3047. > LayoutTests/inspector/debugger/breakpoints/resolved-dump-all-pause-locations-expected.txt:1107 > INSERTING AT: 166:1 > -PAUSES AT: 168:0 > +PAUSES AT: 167:4 > 163 > 164 var v1 = 1, > 165 v2 = 1; > -> 166 l#et l1 = 2, > - 167 l2 = 2; > + => 167 |l2 = 2; > + 168 const c1 = 3, > + 169 c2 = 3; > + 170 Nice!
(In reply to Joseph Pecoraro from comment #5) > Comment on attachment 395495 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395495&action=review > > Do we have a test for something like: > > for(let x = 1, y = 2; ...; ...) {} > > I want to make sure we don't pretty print that poorly, and that we'd pause on `y=2` naturally. We already have pretty printing tests this. I'll add a stepping test. > And maybe just sanity check this as well (though I don't expect it to be any different): > > let x; > if (x = 1, x) {} Good idea :)
Created attachment 395519 [details] Patch
Created attachment 395537 [details] Patch Adjust the tests slightly for a better diff :)
Comment on attachment 395537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395537&action=review Awesome! r=me > Source/JavaScriptCore/parser/Parser.cpp:901 > + head = tail = context.createCommaExpr(headLocation, head); Clever. Maybe too clever. > LayoutTests/inspector/formatting/resources/javascript-tests/for-statements.js:38 > for(var x=1;x<len;++x)1; > -for(var x=1,len=10;x<len;++x)1; > +for(var x=1,len=1;len=10,x<len;len=5,++x)1; FWIW, if you're purely additive to these tests, the diffs end up easier to read and we hypothetically test more cases.
Created attachment 395909 [details] Patch
Committed r259781: <https://trac.webkit.org/changeset/259781> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395909 [details].
<rdar://problem/61496849>