pawn-lang / compiler

Pawn compiler for SA-MP with bug fixes and new features - runs on Windows, Linux, macOS
Other
306 stars 72 forks source link

Another warning 240 false positive. #669

Open Y-Less opened 3 years ago

Y-Less commented 3 years ago

Issue description:

@Daniel-Cortez

https://github.com/pawn-lang/YSI-Includes/blob/cb8a4cd99776b99b0c07933d0be29828c2199a8b/YSI_Storage/y_ini/y_ini_impl.inc#L990

The compiler reports this instance of end as never read, but it might be, depending on what is executed in the loop. It is entirely possible to exit the loop without reassigning end.

There's also a different false positive here:

https://github.com/pawn-lang/YSI-Includes/blob/cb8a4cd99776b99b0c07933d0be29828c2199a8b/YSI_Storage/y_ini/y_ini_impl.inc#L1427

handle is assigned at the end of the case statement, but again this is within a loop and it can be used on later iterations.

Minimal complete verifiable example (MCVE):

Workspace Information:

Y-Less commented 3 years ago
    #pragma unused end

Doesn't seem to bypass the warning. I think it should.

Y-Less commented 3 years ago

OK, I think the first one for end is not applicable, turns out it was right - it was referring to the clobbered write there. The second is still true though.

Daniel-Cortez commented 3 years ago
  #pragma unused end

Doesn't seem to bypass the warning. I think it should.

Of course it wouldn't, you're using it when the warning is already printed. It should work if you place it before the assignment though.

And regarding that false-positive, I'll look into it

Daniel-Cortez commented 3 years ago

MCVE:

main()
{
    for (new x = 5; --x != 0;)
    {
        if (x == 2)
        {
            x = 1;
            goto lbl_cont;
        }
        x = 3; // false-positive warning 240
    lbl_cont:
    }
}

Replacing goto lbl_cont; with continue; and removing the label declaration seems to fix the warning, so I think this might be related to how goto works.

Y-Less commented 3 years ago

Putting it before makes sense. But maybe the message could be improved a bit, because I merged the PR and keep getting caught out. The problem is:

h = 5;
h = 5; // Warning is here.

The warning reads like that second value is the one written and never read, whereas the warning comes when the unread value is clobbered. I understand why, it's just a bit confusing right now.

Daniel-Cortez commented 3 years ago

If I do that, then, for example, if we have this code:

main()
{
    new var1 = 1; // warning 240: assigned value is never used
    new var2 = sizeof(var1); // warning 223: redundant "sizeof": argument size is always 1
    var1 = 2;
    #pragma unused var1, var2
}

the compiler would print warning 240 after warning 223, although the unused assignment happens before the redundant use of sizeof. This is why initially I made it point at the line of the assignment next to the unused one, but now that I think about it, this is indeed counter-intuitive, and having the compiler point at the exact line of the unused assignment might be more convenient, even if the line numbers of multiple warnings come out of order as a result (also, warnings 203 and 204 already work that way, so this isn't something new). Such change might require some ugly workarounds (the line number for the next assignment is stored into sym->lnumber before the variable is checked for the previously assigned value being used, thus clobbering the line number of the previous assignment) but I'll see what I can do about it.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity.