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 compile warnings #728

Merged
merged 31 commits into from
Jul 25, 2024
Merged

Enable compile warnings #728

merged 31 commits into from
Jul 25, 2024

Conversation

barsnick
Copy link
Contributor

Describe your changes

Looking at make VERBOSE=1, I noticed that no compiler warnings were enabled at all. I'm not sure this was intentional. It is certainly not recommended, as warnings can uncover bugs, and also wrong intentions.

This change adds the "simplest" set of warnings with -Wall, but silences warnings for unused functions, as the generated interface implementations are full of them, as they serve as libraries.

No attempt was made yet at -Wextra or at defining a more useful set of warnings.

This CMake change also affects the building of dependencies, and thereby also uncovers warnings in internal dependencies.

The change set further tries to fix some obvious stuff, to make an initial effort of reducing the number of warnings. This is to be considered an example, as more such obvious stuff remains.

This was tested with g++11, and still manages to build the complete project from scratch, albeit with much more noise.

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • I read the contribution documentation and made sure that my changes meet its requirements

@corneliusclaussen
Copy link
Contributor

I like this one, but I should we move it after the June release and get rid of some more of the warnings?

@corneliusclaussen
Copy link
Contributor

Some of the warnings should even be treated as errors I think, e.g. the warning that an enum is not handled in a switch case. This caused quite some trouble before.

@hikinggrass
Copy link
Contributor

I like this one, but I should we move it after the June release and get rid of some more of the warnings?

Sounds like a plan, we had a discussion about it in last week's ci working group meeting and I'm in the process of modifying this PR slightly so it only applies to the targets defined in everest-core. Should be done soon

@barsnick
Copy link
Contributor Author

barsnick commented Jul 2, 2024

@hikinggrass, thanks for the proposed change. I missed that you had pushed it last week. I will rebase the branch and test soon.

@hikinggrass
Copy link
Contributor

@hikinggrass, thanks for the proposed change. I missed that you had pushed it last week. I will rebase the branch and test soon.

I might add a "global compiler options" flag that's off by default as well, so we can have the same result as earlier

@barsnick barsnick force-pushed the cb/enable_and_fix_warnings branch from 5813cbd to 13537f9 Compare July 2, 2024 09:35
@barsnick
Copy link
Contributor Author

barsnick commented Jul 4, 2024

Work for me so far.

I added more fixes. We may want to separate them from this PR?

The libraries also need cmake options to enable compile warnings. These are not covered anymore with the recent change.

Some of the warnings should even be treated as errors I think, e.g. the warning that an enum is not handled in a switch case. This caused quite some trouble before.

This obviously needs some work, since some enums are currently missed (possibly on purpose). Each code owner may need to check. (How about an optional (non-fatal) workflow pipeline which enables compile warnings (later warnings-as-errors?), so that they get flagged.)

@hikinggrass
Copy link
Contributor

hikinggrass commented Jul 4, 2024

I added more fixes. We may want to separate them from this PR?

Might be a good idea to separate them from this PR

The libraries also need cmake options to enable compile warnings. These are not covered anymore with the recent change.

I've just added a new option EVEREST_ENABLE_GLOBAL_COMPILE_WARNINGS that enables the compile warnings globally, off by default

This obviously needs some work, since some enums are currently missed (possibly on purpose). Each code owner may need to check. (How about an optional (non-fatal) workflow pipeline which enables compile warnings (later warnings-as-errors?), so that they get flagged.)

Only issue here is that we would compile twice, might be something to try out though

@corneliusclaussen corneliusclaussen added the post-release Tag that this PR should not go into the current merge window for the upcoming release label Jul 9, 2024
@corneliusclaussen corneliusclaussen removed the post-release Tag that this PR should not go into the current merge window for the upcoming release label Jul 19, 2024
barsnick added 11 commits July 19, 2024 12:18
Currently, no compiler warnings are active at all, which is absolutely not
recommended. Warnings can uncover bugs, but also wrong intentions.

Add "-Wall", but silence warnings for unused functions, as the generated
interface implementations are full of them, as they serve as libraries.

Signed-off-by: Moritz Barsnick <[email protected]>
Re-order an initialization list.

Signed-off-by: Moritz Barsnick <[email protected]>
Re-order an initialization list.
Drop assignment of an unused return value.

Signed-off-by: Moritz Barsnick <[email protected]>
Drop assignment of an unused return value.

Signed-off-by: Moritz Barsnick <[email protected]>
Re-order an initialization list.

Signed-off-by: Moritz Barsnick <[email protected]>
barsnick and others added 10 commits July 19, 2024 12:18
Drop unused variable.

Signed-off-by: Moritz Barsnick <[email protected]>
… if EVEREST_ENABLE_COMPILE_WARNINGS is set to "ON"

By default EVEREST_ENABLE_COMPILE_WARNINGS is set to OFF

Signed-off-by: Kai-Uwe Hermann <[email protected]>
Re-order an initialization list.
Use suggested brackets.
Use valid loop index types.

Signed-off-by: Moritz Barsnick <[email protected]>
Use suggested brackets.

Signed-off-by: Moritz Barsnick <[email protected]>
Fix incorrect intent of `<<` stream vs. fmt::format.

Signed-off-by: Moritz Barsnick <[email protected]>
Use valid loop index types.

Signed-off-by: Moritz Barsnick <[email protected]>
With this option (set to OFF by default) you can globally enable the compile warnings set in the EVEREST_COMPILE_OPTIONS variable

Signed-off-by: Kai-Uwe Hermann <[email protected]>
@corneliusclaussen corneliusclaussen force-pushed the cb/enable_and_fix_warnings branch from 66e0812 to 43f3c61 Compare July 19, 2024 10:18
Copy link
Contributor

@SebaLukas SebaLukas left a comment

Choose a reason for hiding this comment

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

👍

modules/Auth/lib/CMakeLists.txt Outdated Show resolved Hide resolved
@hikinggrass hikinggrass merged commit a874d21 into main Jul 25, 2024
7 of 8 checks passed
@hikinggrass hikinggrass deleted the cb/enable_and_fix_warnings branch July 25, 2024 13:36
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.

5 participants