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

Diagnostics for detection of endless loops #634

Merged
merged 11 commits into from
May 2, 2021

Conversation

Daniel-Cortez
Copy link
Contributor

@Daniel-Cortez Daniel-Cortez commented Jan 30, 2021

What this PR does / why we need it:

This PR implements new diagnostics for detection of endless loops:

  • warning 250: variable "%s" used in loop condition not modified in loop body
    Helps to detects the cases when the user forgot to modify the loop counter variable.
new i = 0;
while (i < 10) // warning 250: variable "i" used in loop condition not modified in loop body
    DoSomething(i);
  • warning 251: none of the variables used in loop condition are modified in loop body
    The same as the previous diagnostic, but works when multiple variables are used inside a loop condition and none of them gets modified.
    For the compiler there's no reasonable way to know which variable should be modified and which shouldn't, which is why I had to create a distinct warning for such situations.
for (new m = 0, n = 10; m < n; ) {} // warning 251: none of the variables used in loop condition are modified in loop body

Which issue(s) this PR fixes:

Fixes #

What kind of pull this is:

  • A Bug Fix
  • A New Feature
  • Some repository meta (documentation, etc)
  • Other

Additional Documentation:

@Daniel-Cortez Daniel-Cortez requested a review from a team as a code owner January 30, 2021 16:11
@Y-Less
Copy link
Member

Y-Less commented Jan 30, 2021

For the compiler there's no reasonable way to know which variable should be modified and which shouldn't, which is why I had to create a distinct warning for such situations.

I don't follow why that needs two warnings. The second warning covers the first case as well, so only 251 is needed.

@Y-Less
Copy link
Member

Y-Less commented Jan 30, 2021

Could I also suggest a test for the case where one loop variable is passed by reference to a function? I know there's no way to determine if that function actually modifies it, but it would just be nice to confirm that the warning isn't given in that case.

@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented Jan 31, 2021

I don't follow why that needs two warnings. The second warning covers the first case as well, so only 251 is needed.

The first one tells which variable exactly needs to be modified. Even when there's only one variable used inside the loop condition, I think it would be more convenient to have that variable pointed at right away than having to inspect the code every time to determine which variable it is. Simply removing warning 250 would mean sacrificing the said convenience.

Another option would be to merge both warnings into one, but they are two completely different messages. I mean, I could do something like this:

/*250*/ "%s modified in loop body"

so warning 250 could be both "variable "%s" used in loop condition not modified in loop body" and "none of the variables used in loop condition are modified in loop body", but currently no other warning or error does this, so I'm not sure if this would be a good solution either.

Could I also suggest a test for the case where one loop variable is passed by reference to a function? I know there's no way to determine if that function actually modifies it, but it would just be nice to confirm that the warning isn't given in that case.

Done.

@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented Jan 31, 2021

Another thing worth mentioning is that warnings 250 and 251 don't work on do-while loops yet, as those need to be handled in a separate way (basically because they have the loop condition written after the loop body), but I'm already working on solving this.

@Daniel-Cortez Daniel-Cortez changed the title Diagnostics for detection of endless loops [WIP] Diagnostics for detection of endless loops Jan 31, 2021
@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented Feb 7, 2021

The new diagnostics seem to work on do-while loops pretty well (just tested this on a few gamemodes, didn't have any false-positives) and pass all tests, so I suppose this PR should be ready now.

@Daniel-Cortez Daniel-Cortez changed the title [WIP] Diagnostics for detection of endless loops Diagnostics for detection of endless loops Feb 7, 2021
@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented May 2, 2021

Amended and force-pushed some of the commits; the changes are the following:

  • Changed the values of flags uLOOPVAR and uNOLOOPVAR from 0x100 and 0x800 to 0x1000 and 0x2000 respectively, in order to avoid a conflict with Fixes for function and variable declarations #631, as value 0x800 is already used for flag uDECLPUBLIC in that PR (also initially value 0x100 was supposed to be used for flag uINITIALIZED in Implement warning 210 #440, so I'm leaving it for when that PR is reimplemented).
  • Fixed pc_loopcond being assigned FALSE instead of 0 in resetglobals().

@Y-Less Y-Less merged commit 6781afe into pawn-lang:dev May 2, 2021
@Daniel-Cortez Daniel-Cortez deleted the loop-diagnostics branch May 4, 2021 17:44
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.

None yet

2 participants