Skip to content

Fixes for #pragma warning #613

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

Merged
merged 6 commits into from
Nov 18, 2020
Merged

Conversation

Daniel-Cortez
Copy link
Contributor

What this PR does / why we need it:

  • Fixes #pragma warning disable not being reset at the end of each compilation pass, resulting in its effects being carried onto the subsequent compilation passes (see #pragma warning #610).
  • Makes the compiler issue an error when #pragma warning push is used more times than #pragma warning pop and vice versa (see #pragma warning #610).
  • Changes the return value type of pc_pushwarnings() and pc_popwarnings() from int to void.
    pc_pushwarnings() never returns FALSE; in case of memory allocation failure it calls error(103), which aborts execution (as 103 is a fatal error), making the line return FALSE; unreachable.
    Also, the values returned by both pc_pushwarnings() and pc_popwarnings() are never used.
  • Covers #pragma warning push/pop/enable/disable with tests (previously there weren't any).

Which issue(s) this PR fixes:

Fixes #610

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 November 12, 2020 12:56
@Y-Less Y-Less merged commit eee2041 into pawn-lang:dev Nov 18, 2020
@Daniel-Cortez
Copy link
Contributor Author

#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.

@Daniel-Cortez Daniel-Cortez deleted the pragma-warning branch November 24, 2020 13:32
@Y-Less
Copy link
Member

Y-Less commented Nov 24, 2020

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
Copy link
Member

Y-Less commented Nov 24, 2020

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

@Y-Less
Copy link
Member

Y-Less commented Nov 24, 2020

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

@Y-Less
Copy link
Member

Y-Less commented Nov 24, 2020

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
Copy link
Contributor Author

Daniel-Cortez commented Nov 28, 2020

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.

@Daniel-Cortez Daniel-Cortez mentioned this pull request May 4, 2021
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.

2 participants