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.

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code is fine

@Andrew350
Copy link

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 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 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
@glx22 glx22 deleted the fix_214 branch May 4, 2021 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue with passing a variable to a function from the PARENT scope
3 participants