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.

Sign up for GitHub

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

Jump to bottom

Fix: [Squirrel] Enforce comma usage in function calls #9398

Closed
wants to merge 1 commit into from
Closed

Fix: [Squirrel] Enforce comma usage in function calls #9398

wants to merge 1 commit into from
+3 −2

Conversation

glx22
Copy link
Contributor

@glx22 glx22 commented Jun 24, 2021

Motivation / Problem

Squirrel errors for call(a, b, c,) with expression expected, found ')', but accepts call(a b c).
I think it's inconsistent, and it should enforce , usage.

Description

Added a check to enforce , in function calls. And found 2 missing ones in regression test :)

I tried to respect Squirrel coding style.

Limitations

Existing AIs may fail to compile if they miss some , in function calls.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain
Copy link
Member

For all the logical reasons this sounds like an extremely bad idea without implementing some versioning / compatibility system :p

@LordAro
Copy link
Member

LordAro commented Jun 24, 2021

Agreed with TB - breaking existing AIs isn't worth it. They're broken enough as it is ;)

Might be worth documenting somewhere though...

@TrueBrain
Copy link
Member

Or make it a developer warning in case it is by accident :)

@glx22
Copy link
Contributor Author

glx22 commented Jun 24, 2021

Indeed compatibility is the biggest issue. And limiting the extra check to >1.12 is quite impossible.

@TrueBrain
Copy link
Member

Each AI tells his compatibility version, so possible it is for sure. The question is, is it worth all that code to basically just scratch an itch you are having ;)

Personally, but this is just my opinion, I agree with LordAro: we should just document this behaviour and leave it for what it is. And documenting it on the wiki alone would be sufficient for me.

@TrueBrain TrueBrain added the candidate: probably not This Pull Request will most likely be closed soon label Jun 26, 2021
@glx22
Copy link
Contributor Author

glx22 commented Jun 26, 2021

I know API version is in ScriptInstance, but it's not accessible from SQCompiler (without changing stuff only for that), then comparing versions needs extra work because it's a string, not a number.

So I agree, it's probably better to discard this PR.

@rubidium42
Copy link
Contributor

Would it be feasible to make it a warning? I'm not sure whether there's infrastructure for that in the code, but it would at least nudge script developers to not add new cases.

@TrueBrain
Copy link
Member

Friendly poke, is a warning possible? Or should we close this PR?

@glx22
Copy link
Contributor Author

glx22 commented Sep 14, 2021

There's no warning support in SQCompiler and errors are handed via exceptions.
So adding warning support just for one use is probably too much work.
Let's just close the PR.

Sorry, something went wrong.

@glx22 glx22 closed this Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
candidate: probably not This Pull Request will most likely be closed soon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@glx22 @TrueBrain @LordAro @rubidium42