Gcc compiler warning flags setting #22
Conversation
| minimal_warnings = ["-Wall", "-Wno-error=deprecated-declarations"], | ||
| strict_warnings = ["-Wextra", "-Wpedantic"], | ||
| minimal_warnings = ["-Wno-error=deprecated-declarations"], | ||
| strict_warnings = ["-Wno-bool-compare"], |
There was a problem hiding this comment.
this does sound very complex, as users would need to figure out in which list which warning is contained.
There was a problem hiding this comment.
Discussion: Disallow disabling minimal_warnings, or any warning from minimal_warnings
| gcc = use_extension("@score_toolchains_gcc//extentions:gcc.bzl", "gcc") | ||
| gcc.extra_features( | ||
| features = [ | ||
| "minimal_warnings", |
There was a problem hiding this comment.
I'm not sure, but is it like this?
| "minimal_warnings", | |
| # minimal_warnings are enabled by default | |
| # strict_warnings are enabled by default | |
| # but for some reason we want to not treat them as errors: |
There was a problem hiding this comment.
I agree . The user does not need to add the features as they are already enabled by default. Nevertheless, he can disable them using the -<feature-name> (e.g. -minimal_warnings)
| "-Wreorder", | ||
| "-Wnarrowing" | ||
| ], | ||
| "treat_warnings_as_errors": [] |
There was a problem hiding this comment.
Does the empty list here mean that -Werror is set? I am assuming we want errors as warnings on, and then I guess -Werror should be in this empty list?
There was a problem hiding this comment.
-Werror is not set by default.
treat_warnings_as_errors is empty so that warnings are not treated as errors by default. I did not want that during integration ( merge with main) that consumers are getting build errors for every warning they have.
It is also possible to enable -Werror by default and consumers disable the features using -treat_warnings_as_errors.
what do you think?
There was a problem hiding this comment.
At least "-Wno-error=deprecated-declarations" should be set here because the whole point of deprecations is to enable a smooth transition and not to break anyones build.
|
Nice, did you already try them out with lola or baselibs? |
| "-Wreturn-type", | ||
| "-Wcomment" | ||
| ], | ||
| "strict_warnings": [ |
There was a problem hiding this comment.
stricts sound like only the nerds would apply that. I'd rename this to recommended.
| "strict_warnings": [ | |
| "recommended_warnings": [ |
There was a problem hiding this comment.
I'm against renaming this. Maybe that outs me as a "nerd", but then I'm proud to be a safety-nerd. 😉
"strict_warnings" means that this is the strict set of compiler warnings that we as a project want to enforce.
In comparison "recommended" does not give you any knowledge why we recommend it.
Based on the scenario "recommended" may also have very different results.
For example I would recommend different compiler warnings for QM and ASIL-B code.
TLDR: "Recommended" is too ambiguous. Please stick with the current naming.
There was a problem hiding this comment.
If "strict" is to be enforced, it should be moved to "minimal". Or do we want three groups, minimal + strict + recommended?
@FScholPer : yes, I tried it with baselibs repo for the bazel targets //score/datetime_converter and //score/filesystem and I can see some warnings. |
| DEFAULT_FEATURES = [ | ||
| "minimal_warnings", | ||
| "strict_warnings", | ||
| "treat_warnings_as_errors", |
There was a problem hiding this comment.
for now: all minimal_warnings should be errors from day 0. Then we follow up with the others.
NEOatNHNG
left a comment
NEOatNHNG
left a comment
There was a problem hiding this comment.
-Wno-error=deprecated-declarations needs more consideration IMHO. Normally it should be set whenever you set -Werror
There was a problem hiding this comment.
Shouldn't this directory be renamed as extensions (typo)? IMHO we should do that ASAP before we get more code with the wrong spelling.
| "-Wreorder", | ||
| "-Wnarrowing" | ||
| ], | ||
| "treat_warnings_as_errors": [] |
There was a problem hiding this comment.
At least "-Wno-error=deprecated-declarations" should be set here because the whole point of deprecations is to enable a smooth transition and not to break anyones build.
| gcc.warning_flags( | ||
| minimal_warnings = ["-Wall", "-Wno-error=deprecated-declarations"], | ||
| strict_warnings = ["-Wextra", "-Wpedantic"], | ||
| minimal_warnings = ["-Wno-error=deprecated-declarations"], |
There was a problem hiding this comment.
Why is "-Wno-error=deprecated-declarations" even mentioned here? Normally it only takes any effect if you also set -Werror
6 participants
Related issue: Define compiler flags for gcc compiler
This PR centralizes GCC warning flags configuration for improved consistency, maintainability, and reuse across the SCORE Project modules.
By default, the toolchain activates the following features using predefined compiler flags:
minimal_warnings(e.g., -Wall, -Wextra)strict_warnings(e.g., -Wpedantic)treat_warnings_as_errors(e.g., -Werror)Consumers can disable features via
-<feature_name>and customize behavior with additional flags or-Wno-options.