search

dvidelabs / flatcc

FlatBuffers Compiler and Library in C for C
Apache License 2.0
632 stars 180 forks source link

Diagnostic flags aren't properly restored #205

Closed codido closed 2 years ago

codido commented 2 years ago

The diagnostic flags are pushed and popped by include/flatcc/portable/pdiagnostic_push.h & include/flatcc/portable/pdiagnostic_pop.h respectively. However, due to their include guards (PDIAGNOSTIC_PUSH_H and PDIAGNOSTIC_POP_H), this can only work once in each compile unit. Since the prologue and epilogue headers may be included more than once (e.g., common reader and generated flatbuffers, or more than one generated header at a time), the diagnostic changes may affect the parent file.

mikkelfj commented 2 years ago

That should not be he case. Push and Pop should be outside the include guards, AFAIK there is a comment about that somewhere. Otherwise it is a regression. Note that macros are defined within include guards, but pdiagnostic.h that uses the macros is included outside of the include guards.

However, for some compilers (looking at you MSVC), push and pop is not really supported, and here things work a bit differently.

Do you experience a problem, or can you be more specific about the issue in case I am missing something?

codido commented 2 years ago

Thanks for the quick reply!

While pdiagnostic.h isn't included inside the include guard, the diagnostic push & pop pragma are. As a result, the warnings changes will take effect, but will not be saved & restored more than once.

For instance, with the current code the project will build (with clang) without any warning, even though there should be some unused variables/functions such as the function argument here: https://github.com/dvidelabs/flatcc/blob/c6c2064ec1f1eb8e9734c7d3a1105fa6102eba9a/test/monster_test/monster_test.c#L97

mikkelfj commented 2 years ago

You are correct. I will classify this as a brainfart especially since I was aware of the issue when writing the code. Should be fixed now. Please check on master.

codido commented 2 years ago

Excellent, thank you for the quick fix!

By the way, you might want to amend the comment in pdiagnostic_push.h regarding the include guard, but the fix works, thanks.

Guess we can close this issue?

mikkelfj commented 2 years ago

Removed comment. I wonder if I actually intended it to work as it did? Anyway, this should be better, I hope.