Conversation
saelo
left a comment
saelo
left a comment
There was a problem hiding this comment.
Thanks a lot! Did a first round of review now
| for _ in 0..<n { | ||
| b.loadString(b.randomString()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Thanks a lot for your suggestion, and my apologies for the bad code style.
| final class EndTryCatchFinally: JsOperation { | ||
| override var opcode: Opcode { .endTryCatchFinally(self) } | ||
|
|
||
| } | ||
| } | ||
|
|
||
| final class LoopNestedContinue: JsOperation { |
There was a problem hiding this comment.
Maybe here would be a good place for a short comment describing how this operation works, in particular how depth is interpreted (e.g. that it wraps around if there are fewer open loops)
| w.emitBlock("while (\(COND)) {") | ||
| w.enterNewBlock() | ||
|
|
||
|
|
There was a problem hiding this comment.
More whitespace here and below
|
|
||
| var w = JavaScriptWriter(analyzer: analyzer, version: version, stripComments: !options.contains(.includeComments), includeLineNumbers: options.contains(.includeLineNumbers)) | ||
|
|
||
| var loopLabelStack = LabelStack(type: .loopblock) |
There was a problem hiding this comment.
Can we assert at the end of the lifting that this stack is now empty? Just to catch any cases where we forget the pop()
|
|
||
| enum LabelType { | ||
| case loopblock | ||
| case ifblock |
There was a problem hiding this comment.
I guess for this PR, only loopBlock would be supported? The others should probably move to the corresponding PR
| } | ||
| } | ||
|
|
||
| mutating func translateBreakLabel(w: inout ScriptWriter, expDepth: Int){ |
There was a problem hiding this comment.
Nit: missing space before { (also below)
| } | ||
|
|
||
| mutating func insert(_ pos: Int, _ content: String){ | ||
| if code.index(code.startIndex, offsetBy: pos, limitedBy: code.endIndex) != nil { |
There was a problem hiding this comment.
Can this ever not be the case? Wouldn't that be a bug?
| } | ||
| } | ||
|
|
||
| func testLoopNestedContinueLifting(){ |
There was a problem hiding this comment.
I'm wondering, does the label insertion work correctly if we also print line numbers? Did this test:
ever fail? Maybe you could add an extra test like this one that inserts labels but also prints line numbers?There was a problem hiding this comment.
If this change breaks line numbering, maybe we could just add the label to the same line as the loop begin? So lift to something like label1: for (...) { instead?
| } | ||
|
|
||
| /// A structure that manages label positions within a specific control flow block (e.g., loop, if, switch, etc.). | ||
| public struct LabelStack { |
There was a problem hiding this comment.
I wonder if this struct should actually be part of the ScriptWriter or the JavaScriptWriter itself. It feels a bit awkward that you need to pass in the ScriptWriter or internal state of the script writer (the current position). I think this would all go away if the ScriptWriter/JavaScriptWriter itself maintained this stack. And I think it would also remove the need for the withScriptWriter method above. Would that work? That way, the ScriptWriter also wound't need to expose the insert method which probably shouldn't be used for any other purposes.
Yi2255
force-pushed
the
feature-loopNestedContinue
branch
from
17edc6b to
e0e81c9
Compare
2 participants
Hi Saelo,
First of all, I sincerely apologize for the long delay regarding the implementation of
LabelStatement.Following your suggestion #479 , I have split this feature into two separate PRs,
LoopNestedContinueandNestedBreak.This PR implements
LoopNestedContinue. As discussed in #479, I add a new JsOperationloopNestedContinueandand a new CodeGeneratorLoopNestedContinueGenerator. Beside, the code design inJavaScriptLifter.swiftis as follows:LabelStackto trace the nested code block.LabelStackisLabelPin, which has two fields:beginPosandhasLabel.beginPosis the index of the starting point of the js code block, andhasLabelindicates whetherbeginPoshas generated a label (with a small probability of generating N lines of break or continue in the same block).continueNestedappears. Therefore, the core operation ofLabelStackisinsertLabel, which has three operations: ① Insert label string into js code; ② mark that a label has been inserted here; ③Move the positions of all LabelPins behind the stack at the corresponding depth back by the corresponding size.Please let me know if any changes are needed; Thanks a lot!