Skip to content
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

Merged
merged 38 commits into from
Mar 20, 2024
Merged

Conversation

erlingrj
Copy link
Collaborator

@erlingrj erlingrj commented Mar 6, 2024

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:

- 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
@erlingrj erlingrj marked this pull request as draft March 6, 2024 14:52
@erlingrj erlingrj marked this pull request as ready for review March 8, 2024 15:59
core/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@lhstrh lhstrh left a 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.

core/federated/RTI/rti_remote.c Outdated Show resolved Hide resolved
core/federated/RTI/rti_remote.c Outdated Show resolved Hide resolved
core/threaded/scheduler_NP.c Show resolved Hide resolved
include/core/federated/network/net_common.h Outdated Show resolved Hide resolved
lingua-franca-ref.txt Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@erlingrj erlingrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Marten

core/CMakeLists.txt Outdated Show resolved Hide resolved
core/threaded/scheduler_NP.c Show resolved Hide resolved
include/core/federated/network/net_common.h Outdated Show resolved Hide resolved
lingua-franca-ref.txt Outdated Show resolved Hide resolved
@erlingrj
Copy link
Collaborator Author

@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).

Copy link
Contributor

@edwardalee edwardalee left a 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.

core/lf_utils.cmake Show resolved Hide resolved
core/tracepoint.c Show resolved Hide resolved
Copy link
Contributor

@petervdonovan petervdonovan left a 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.

core/CMakeLists.txt Show resolved Hide resolved
core/federated/federate.c Outdated Show resolved Hide resolved
core/tracepoint.c Show resolved Hide resolved
Copy link
Collaborator Author

@erlingrj erlingrj left a 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.

@erlingrj erlingrj added this pull request to the merge queue Mar 20, 2024
Merged via the queue into main with commit 75fcdba Mar 20, 2024
30 checks passed
@erlingrj erlingrj deleted the warnings-as-errors branch May 6, 2024 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants