Skip to content
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 #214: Prevent scope change for function calls when optimising switches #216

Merged
merged 1 commit into from May 4, 2021

Conversation

@glx22
Copy link
Contributor

@glx22 glx22 commented May 4, 2021

Fixes #214

A procedure call can use variables from current scope as call parameters.
When the switch is optimised as a procedure call, a return action in caller's scope is created, and wrong variables may be accessed.

Prevent this by considering procedure calls like any complex expression, and allow the switch to be rewritten.
It's not needed for random switches as variables are always accessed in SELF scope for them.
Don't optimise procedure calls in random switches.

And while I was updating a regression test, I decided to include string test for #205.

@glx22 glx22 force-pushed the glx22:fix_214 branch from f660a94 to 8ba8f95 May 4, 2021
@LordAro
LordAro approved these changes May 4, 2021
Copy link
Member

@LordAro LordAro left a comment

Code is fine

@Andrew350
Copy link

@Andrew350 Andrew350 commented May 4, 2021

So, I managed to frankenstein this PR together with current master and PR #173, and it appears to work, so thanks for a quick fix 🙂. One thing I noticed: The .grf works as expected and I get no errors when using the switch in this format:

switch (FEAT_INDUSTRYTILES, PARENT, switch_mineral_mine_prod_change_2, switch_primary_industry_prod_change(transported_last_month_pct("MNRL"))) {
	return;
}

However if I write it similar to the way shown in the original issue, but with the 4th parameter simply empty brackets and the call as the return value...

switch (FEAT_INDUSTRYTILES, PARENT, switch_mineral_mine_prod_change_2, []) {
	switch_primary_industry_prod_change(transported_last_month_pct("MNRL"));
}

...I don't get any syntax errors but nmlc still "optimises" the switch resulting in failure. I'm not really sure if this is even valid code, or related to this issue, but thought I should mention it.

@glx22
Copy link
Contributor Author

@glx22 glx22 commented May 4, 2021

Does it happen with or without my fix ?
With my fix the second version is rewritten to the first version during optimisation.
Without my fix the second version is translated in a return action (same format as first version, but in caller's scope).

@Andrew350
Copy link

@Andrew350 Andrew350 commented May 4, 2021

This is with your fix applied, but also PR #173 applied as well (as I can't build without it). I'd have to build a special GRF to check your fix separately, but they don't seem to conflict here so not sure if that affects anything.

EDIT: Actually disregard that, I tested again and it is working after fixing a typo in my code, so it's all good 👍

@glx22 glx22 merged commit 8414cc4 into OpenTTD:master May 4, 2021
21 checks passed
21 checks passed
@github-actions
Commit checker
Details
@github-actions
Python 3.5 on ubuntu-latest
Details
@github-actions
Security and Quality Security and Quality
Details
@github-actions
Python 3.6 on ubuntu-latest
Details
@github-actions
Python 3.7 on ubuntu-latest
Details
@github-actions
Python 3.8 on ubuntu-latest
Details
@github-actions
Python pypy3 on ubuntu-latest
Details
@github-actions
Python 3.5 on macOS-latest
Details
@github-actions
Python 3.6 on macOS-latest
Details
@github-actions
Python 3.7 on macOS-latest
Details
@github-actions
Python 3.8 on macOS-latest
Details
@github-actions
Python 3.5 on windows-2016
Details
@github-actions
Python 3.6 on windows-2016
Details
@github-actions
Python 3.7 on windows-2016
Details
@github-actions
Python 3.8 on windows-2016
Details
@github-actions
Python 3.x on ubuntu-latest
Details
@github-actions
Python 3.x on macOS-latest
Details
@github-actions
Python 3.x on windows-2016
Details
@github-actions
Flake8
Details
@github-actions
Black
Details
@github-code-scanning
CodeQL No new or fixed alerts
Details
@glx22 glx22 deleted the glx22:fix_214 branch May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants