closureiters: use case-based dispatcher
#607
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zerbina
added
enhancement
zerbina
force-pushed
the
closureiters-case-statement-dispatcher
branch
from
5fc978a
to
e04de01
Compare
zerbina
changed the title
case-based dispatcher
saem
approved these changes
Mar 29, 2023
| @@ -185,7 +185,7 @@ type | |||
| unrollFinallySym: PSym # Indicates that we're unrolling finally states (either exception happened or premature return) | |||
| curExcSym: PSym # Current exception | |||
|
|
|||
| states: seq[PNode] # The resulting states. Every state is an nkState node. | |||
| states: seq[tuple[label: int, body: PNode]] # The resulting states | |||
There was a problem hiding this comment.
This type reads more nicely and the tuple fields make the access below easier to follow when skimming. 👍🏽
compiler/sem/closureiters.nim
Outdated
| while i < ctx.states.len - 1: | ||
| let fs = skipStmtList(ctx, ctx.states[i][1]) | ||
| if fs.kind == nkGotoState and i != 0: | ||
| if ctx.states[i].label == -1: # is it unused? |
There was a problem hiding this comment.
Very soft suggestion: make -1 a constant, such as unusedStateIdx, might read a bit better. It'd also be applicable to the setting of the label values above.
There was a problem hiding this comment.
That's much better, thanks.
|
I definitely like how this collapses the language into itself. Even before this change, I can imagine using case to explain how iterators work to a newcomer, and now the simple explanation and implementation are that much closer. Makes bridging the gap from language user to contributor that much smaller. 🚀 |
zerbina
force-pushed
the
closureiters-case-statement-dispatcher
branch
from
e04de01
to
c5fc991
Compare
Summary
=======
Use a case statement for the dispatcher generated by the `closureiters`
transformation, removing the need for both dedicated code generator
support and the `nkState` node kind. This is a significant step towards
closure iterator support the JavaScript and VM backend.
While neither officially supported nor documented, it was previously
possible to emit `nkGotoState` and `nkState` nodes from a macro, which
could be used for unsafe unstructured control-flow. With the removal of
the `nkState` node kind and `nkGotoState` being disallowed in AST
reaching semantic analysis, the aforementioned is no longer possible.
Details
=======
The AST generated by the `closureiters` transformation always looked
as follows:
```nim
while true:
block :stateLoop: # the block's body is wrapped in a try-except, if
# necessary
goto env.:state # goto the label corresponding to the run-time value
# of `:state`
# declarations
body
```
where `body` defines the code blocks associated with each state
value, like so:
```nim
state 0:
...
env.:state = 1
break :stateLoop
state 1:
...
state 2:
...
... # more labels (if used) follow
```
The control-flow described by the `goto` (`nkGotoState`) followed by
the labels (`nkState`) is equally representable by the use of a case
statement, which is what is now done after this commit.
To summarize the change:
- don't support `nkGotoState` nodes in AST reaching sem
- adjust `closureiters` to generate a case statement for the dispatcher
and to not depend on `nkState`
- remove C code-generator support for `nkGotoState` and `nkState`
- remove the `nkState` node kind
Nodes with the `nkGotoState` kind now only exists *during* the
transformation, but never before or after.
|
bors r+ |
|
Build succeeded: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Use a case statement for the dispatcher generated by the
closureiterstransformation, removing the need for both dedicated code generator support and thenkStatenode kind. This is a significant step towards closure iterator support the JavaScript and VM backend.While neither officially supported nor documented, it was previously possible to emit
nkGotoStateandnkStatenodes from a macro, which could be used for unsafe unstructured control-flow. With the removal of thenkStatenode kind andnkGotoStatebeing disallowed in AST reaching semantic analysis, the aforementioned is no longer possible.Details
The AST generated by the
closureiterstransformation always looked as follows:where
bodydefines the code blocks associated with each state value, like so:The control-flow described by the
goto(nkGotoState) followed by the labels (nkState) is equally representable by the use of a case statement, which is what is now done after this commit.To summarize the change:
nkGotoStatenodes in AST reaching semclosureitersto generate a case statement for the dispatcher and to not depend onnkStatenkGotoStateandnkStatenkStatenode kindNodes with the
nkGotoStatekind now only exists during the transformation, but never before or after.Notes for Reviewers
jsgenandvmgendon't implement themFinishedmagic yet, meaning that.closureiterators are still not supported by the respective backendsmFinishedsupport as a follow-up PR