-
Notifications
You must be signed in to change notification settings - Fork 24
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
Enable all warnings and treat them as errors #387
Conversation
- Move linking with threading library to low-level platform with PRIVATE - Remove the wierd REACTORC_COMPILE_DEFS variable - Dont add source files when they are not to be compiled - Add custom function for enabling warnings and -Werror
- printf %p must be casted to void* - comparison between signed and unsigned - unused function arguments - remove the empty trace macros - Add wrapper thread function for C11
…or-c into warnings-as-errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These look like great cleanups to me. Left some minor comments/questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Marten
@lhstrh and @edwardalee, I propose we merge this next so that the other PRs are forced to handle their warnings (and not having me do it afterwards in this PR). One change that I would like an extra set of eyes on is to the lf_thread_create in the C11 support. To avoid compiler warnings I had to define a wrapper function with a signature expected by the C11 (needs to return an int). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look great to me! Only one question in the comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a quick look and don't have any important concerns. The change in the C11 threads support looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a final self-review, merging this now.
In this PR I enable all warnings and treat them as errors with
-Wall -Wpedantic -Werror
, this is how it is done in reactor-cpp. This triggered some refactoring of the CMake system and addressing a lot of warnings all over the code-base. I expect there to be a lot more warnings when CI run tests on Windows, macOS, Zephyr etc.The advantage of getting this merged is that it puts more requirements on the code that is checked in. The next step is to add clang-tidy which performs static analysis and catches even more possible undefined behaviour.
Disabled on the following platforms: