-
Notifications
You must be signed in to change notification settings - Fork 250
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
Goto crossing scopes: fix scope tree entry of conditions #8187
base: develop
Are you sure you want to change the base?
Conversation
1b58d18
to
ecc35f2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8187 +/- ##
===========================================
+ Coverage 78.37% 78.38% +0.01%
===========================================
Files 1721 1720 -1
Lines 188083 187983 -100
Branches 18470 18468 -2
===========================================
- Hits 147404 147346 -58
+ Misses 40679 40637 -42 ☔ View full report in Codecov by Sentry. |
ecc35f2
to
5108c55
Compare
I find it odd that the order of conversion matters? |
I'm with you in that I don't like that the order matters, but I can't claim to have a better solution to offer: the order (nowadays) matters, because we keep track of variable declarations in the scope stack (which now is a scope tree). That is, conversion has that side effect of updating the scope tree. In absence of ideas on how to avoid this side effect I'd maintain that this PR here is the correct fix. |
I'd really like to get rid of that side effect. If need be, I'd prefer to pass around that scope stack. |
With apologies for taking this long to get back to this: I am still trying to figure out whether this can be done without making all of |
Do we want to do this in the C front-end instead? That's where scoping belongs. |
I equally thought that was the right choice. The challenge, however, is with goto instructions, where we’d need a frontend (codet) level control-flow graph. But maybe we should bite that bullet, it would also help with always_inline? |
5108c55
to
2fc75f9
Compare
2fc75f9
to
4d75a8d
Compare
4d75a8d
to
6ffa1c5
Compare
We must convert the condition of an if/then/else first to make sure any declarations produced by converting the condition have a suitable scope-tree node. Previously, gotos in either branch of the if-then-else were led to believe that declarations resulting from condition were not yet in scope. The feature from diffblue#8091 would, thus, result in a spurious loop back to the declaration to put it in scope before following the goto edge.
6ffa1c5
to
4827dd4
Compare
We must convert the condition of an if/then/else first to make sure any declarations produced by converting the condition have a suitable scope-tree node. Previously, gotos in either branch of the if-then-else were led to believe that declarations resulting from condition were not yet in scope. The feature from #8091 would, thus, result in a spurious loop back to the declaration to put it in scope before following the goto edge.