-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Yield in try #7770
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
Yield in try #7770
Conversation
|
I ripped out the try transformation from the |
|
@yglukhov btw, will |
|
@dom96, of course, it is in the tests already :) |
|
Known issues fixed. Please proceed with the review. |
|
The compiler doesn't crash anymore, but the async tests don't work with your transformation: https://github.com/nim-lang/Nim/tree/rip-out-try-async-2 |
|
@dom96, I've fixed one of the tests. The rest you should fix along with your transformation rip out. |
|
So ... is this ready to be merged? |
compiler/closureiters.nim
Outdated
| needsSplit = true | ||
|
|
||
| result = newNodeI(nkStmtListExpr, n.info) | ||
| if n.typ.isNil: internalError("lowerStmtListExprs: constr typ.isNil") |
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.
Pretty sure nkArgList has no type.
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.
Ok, i just removed nkArgList from the case branch, since its not supposed to make it into the transformation anyway.
compiler/closureiters.nim
Outdated
|
|
||
| ctx.curExcHandlingState = exceptIdx | ||
|
|
||
| discard ctx.transformReturnsInTry(tryBody) |
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.
These discards are concerning. The result is not to be discarded unless ctx.transformReturnsInTry(tryBody) == tryBody. If so, this should be asserted.
fixes #4695 ref #15818 Since `nkState` is only for the main loop state labels and `nkGotoState` is used only for dispatching the `:state` (since #7770), it's feasible to rewrite the loop body into a single case-based dispatcher, which enables support for JS, VM backend. `nkState` Node is replaced by a label and Node pair and `nkGotoState` is only used for intermediary processing. Backends only need to implement `nkBreakState` and `closureIterSetupExc` to support closure iterators. pending #23484 <del> I also observed some performance boost for C backend in the release mode (not in the danger mode though, I suppose the old implementation is optimized into computed goto in the danger mode) </del> allPathsAsgnResult???
Includes #7704.
Adds yield in try.