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

Fixes for `#pragma warning` #613

Closed Daniel-Cortez closed 3 years ago

Daniel-Cortez commented 3 years ago

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #610

What kind of pull this is:

Additional Documentation:

Daniel-Cortez commented 3 years ago
#if defined ENABLE_ALL_WARNINGS
#pragma warning pop
#endif

It seems that you're completely missing the point on how #pragma warning push/pop works.

Firstly, #pragma warning pop doesn't enable all warnings, it reverts them to the state previously saved with #pragma warning push.

Secondly, if you thought #pragma warning pop would simply enable all warnings if there was no matching #pragma warning push, then again, no, it doesn't do anything in such cases. For example, try compiling this code with the v3.10.10 release:

#pragma warning disable 203
#pragma warning pop
new var;
main(){}

In this example the compiler wouldn't warn about var being unused, because warning 203 is disabled and #pragma warning pop doesn't actually do anything when the warnings stack is empty (no prior #pragma warning push).

Thirdly, usually an unmatched pop is a sign that the user forgot to use #pragma warning push somewhere, which is why I made the compiler issue an error in such cases, to inform the user about their mistake instead of silently ignoring the problem.

Y-Less commented 3 years ago

TBH I was unsure about some of these things, but pop means "restore the last item". If there's no item to restore, what should happen?

Y-Less commented 3 years ago

Nothing wrong with complex sets of conditional pushes/pops, as long as they match up in the end.

Y-Less commented 3 years ago

If you don't want to pop, just don't push. Disabling a warning doesn't require a push first.

Y-Less commented 3 years ago

But that code is probably a mistake. Maybe it should be a warning instead, but still. The pop will only restore the most recent push, but there are three pushes and three warning disables there. So in that code after the pop only warning 215 will be restored. You'd have to do:

#pragma warning pop
#pragma warning pop
#pragma warning pop

To restore all three.

Daniel-Cortez commented 3 years ago

Maybe it should be a warning instead, but still.

Well, as I already mentioned in #610, the main reason I made them as errors was because I wanted unmatched #pragma warning push/pop to follow the same rules that already apply to unmatched #ifs and #endifs. I wouldn't mind changing them into warnings though, but that would require adding at least 1 new warning for this, and there's already a PR that implements a new warning (614), which probably needs to be merged in first in order to avoid possible merge conflicts.