-
Notifications
You must be signed in to change notification settings - Fork 379
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
fix: re-create blocks on for loop iterations #1768
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1768 +/- ##
==========================================
+ Coverage 47.50% 47.95% +0.44%
==========================================
Files 388 441 +53
Lines 61375 65211 +3836
==========================================
+ Hits 29159 31273 +2114
- Misses 29774 31333 +1559
- Partials 2442 2605 +163
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The main issue I see with this is that it will affect performance for every for loop iteration. |
Closing for house-keeping and seeing as you made another PR |
My PR isn't good, but found a better way which we talked about in the call.
|
Closing in favour of other PRs. |
This is an attempt at an alternative to fix #1135. See a current solution at #1585.
The solution is simple: for all our for loops, whenever we reach the end of its body and are about to go back at the top, re-create their Blocks from scratch, copying over the
bodyStmt
. In the case of three-clause for loops, copy over any current values of the declared initialization variables. This is consistent with the behaviour as of Go 1.22.This fix is "dumb", for now. I'm pretty sure re-creating a Block on each iteration is not the best thing for performance, and this may be a step that we can try to "optimize out" if the for loop doesn't generate a closure (as most for loops are). However, it is a very small patch, and I'd like to gauge whether we like this fix and should adopt some variation of it or not.
Credit to maxwell for the tests added. (Note: the
goto
tests still don't work; however, I think the fix for thegoto
tests involves correctly "understanding" the difference between assignment/defintiion within the GnoVM, and as such are out of scope here. Naturally, if there is an alternative fix that gets within the same patch to fix this issue and the goto issue, I'm all ears)cc/ @jaekwon @deelawn @ltzmaxwell