-
Notifications
You must be signed in to change notification settings - Fork 11
Gcc compiler warning flags setting #22
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,32 +28,35 @@ toolchain_gcc_dependencies() | |
| ## Configuring the toolchain to a project needs | ||
| The `score_toolchains_gcc` module currently supports only GNU GCC version **12.2.0**. Support for multiple GCC versions is planned, with future versions expected to be selectable via an extended toolchain interface. As of now, version 12.2.0 is the sole supported target. | ||
|
|
||
| The module exposes an API that allows consumers to enable or disable specific toolchain behaviors related to compilation diagnostics. The following features can be individually configured: | ||
| The module exposes an API that allows consumers to enable or disable specific toolchain behaviors related to compilation diagnostics. | ||
| By default, the toolchain activates the following features using predefined compiler flags: | ||
|
|
||
| 1. Minimal warning flags — enables a basic set of compiler warnings | ||
| 2. Strict warning flags — enables a more aggressive set of warnings (e.g., `-Wall, -Wextra`) | ||
| 1. Minimal warning flags — enables a basic set of compiler warnings (e.g., `-Wall, -Wextra`) | ||
| 2. Strict warning flags — enables a more aggressive set of warnings (e.g., `-Wpedantic`) | ||
| 3. A `treat warnings as errors` flags — promotes all warnings to errors (e.g., `-Werror`) | ||
|
|
||
| These features provide fine-grained control over the compiler's warning policy. If no features are explicitly selected, the toolchain will apply no additional warning-related flags by default. | ||
| Consumers can disable any of these features by prefixing the feature name with a dash (e.g., `-minimal_warnings`). When a feature is disabled this way, neither the default nor any user-defined flags associated with it will be applied. | ||
| Additionally, consumers can adjust the compiler's warning behavior by adding custom flags or disabling specific warnings using the `-Wno-` prefix. | ||
| These features provide fine-grained control over the compiler's warning policy. | ||
|
|
||
| To set wanted flags, the following API needs to be used: | ||
| ```python | ||
| gcc = use_extension("@score_toolchains_gcc//extentions:gcc.bzl", "gcc") | ||
| gcc.extra_features( | ||
| features = [ | ||
| "minimal_warnings", | ||
| "treat_warnings_as_errors", | ||
| "-treat_warnings_as_errors", | ||
| ], | ||
| ) | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is |
||
| strict_warnings = ["-Wno-bool-compare"], | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this does sound very complex, as users would need to figure out in which list which warning is contained.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussion: Disallow disabling minimal_warnings, or any warning from minimal_warnings |
||
| treat_warnings_as_errors = ["-Werror"], | ||
| ) | ||
| use_repo(gcc, "gcc_toolchain", "gcc_toolchain_gcc") | ||
| ``` | ||
| * `extra_features` - This will enable all features which are set in the list. | ||
| * `warning_flags` - This will set flags for selected features. | ||
| * `extra_features` - Enables or disables features listed by the consumer. | ||
| * `warning_flags` - Sets compiler flags for the features that are currently enabled. | ||
|
|
||
| ### Using WORKSPACE file | ||
| The same approuch needs to be done when configuring toolchain over WORKSPACE file: | ||
|
|
@@ -64,11 +67,11 @@ gcc_toolchain( | |
| gcc_repo = "gcc_toolchain_gcc", | ||
| extra_features = [ | ||
| "minimal_warnings", | ||
| "treat_warnings_as_errors", | ||
| "-treat_warnings_as_errors", | ||
| ], | ||
| warning_flags = { | ||
| "minimal_warnings": ["-Wall", "-Wno-error=deprecated-declarations"], | ||
| "strict_warnings": ["-Wextra", "-Wpedantic"], | ||
| "minimal_warnings": ["-Wno-error=deprecated-declarations"], | ||
| "strict_warnings": ["-Wno-bool-compare"], | ||
| "treat_warnings_as_errors": ["-Werror"], | ||
| }, | ||
| ) | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this directory be renamed as |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -21,9 +21,76 @@ def _gcc_impl(mctx): | |||||
| if not mod.is_root: | ||||||
| fail("Only the root module can use the 'gcc' extension") | ||||||
|
|
||||||
| DEFAULT_FEATURES = [ | ||||||
| "minimal_warnings", | ||||||
| "strict_warnings", | ||||||
| "treat_warnings_as_errors", | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for now: all minimal_warnings should be errors from day 0. Then we follow up with the others. |
||||||
| ] | ||||||
|
|
||||||
|
|
||||||
| DEFAULT_WARNING_FLAGS = { | ||||||
| "minimal_warnings": [ | ||||||
| "-Wall", | ||||||
| "-Wextra", | ||||||
| "-Wundef", | ||||||
| "-Wwrite-strings", | ||||||
| "-Wpointer-arith", | ||||||
| "-Wcast-align", | ||||||
| "-Wshadow", | ||||||
| "-Wswitch-bool", | ||||||
| "-Wredundant-decls", | ||||||
| "-Wswitch-enum", | ||||||
| "-Wtype-limits", | ||||||
| "-Wformat", | ||||||
| "-Wformat-security", | ||||||
| "-Wconversion", | ||||||
| "-Wlogical-op", | ||||||
| "-Wreturn-local-addr", | ||||||
| "-Wunused-but-set-variable", | ||||||
| "-Wunused-parameter", | ||||||
| "-Wunused-but-set-parameter", | ||||||
| "-Wunused-variable", | ||||||
| "-Wunused", | ||||||
| "-Wparentheses", | ||||||
| "-Wuninitialized", | ||||||
| "-Wsequence-point", | ||||||
| "-Wsign-compare", | ||||||
| "-Wignored-qualifiers", | ||||||
| "-Wcast-qual", | ||||||
| "-Wreturn-type", | ||||||
| "-Wcomment" | ||||||
| ], | ||||||
| "strict_warnings": [ | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. stricts sound like only the nerds would apply that. I'd rename this to recommended.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. TLDR: "Recommended" is too ambiguous. Please stick with the current naming.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If "strict" is to be enforced, it should be moved to "minimal". Or do we want three groups, minimal + strict + recommended? |
||||||
| "-Wpedantic", | ||||||
| "-Wbad-function-cast", | ||||||
| "-Wstrict-prototypes", | ||||||
| "-Wodr", | ||||||
| "-Wlogical-not-parentheses", | ||||||
| "-Wsizeof-array-argument", | ||||||
| "-Wbool-compare", | ||||||
| "-Winvalid-pch", | ||||||
| "-Wformat=2", | ||||||
| "-Wmissing-format-attribute", | ||||||
| "-Wformat-nonliteral", | ||||||
| "-Wformat-signedness", | ||||||
| "-Wvla", | ||||||
| "-Wuseless-cast", | ||||||
| "-Wdouble-promotion", | ||||||
| "-Wmissing-prototypes", | ||||||
| "-Wreorder", | ||||||
| "-Wnarrowing" | ||||||
| ], | ||||||
| "treat_warnings_as_errors": [] | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It is also possible to enable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At least |
||||||
| } | ||||||
|
|
||||||
|
|
||||||
| toolchain_info = None | ||||||
| features = [] | ||||||
| warning_flags = None | ||||||
| features = list(DEFAULT_FEATURES) | ||||||
| warning_flags = { | ||||||
| "minimal_warnings": list(DEFAULT_WARNING_FLAGS["minimal_warnings"]), | ||||||
| "strict_warnings": list(DEFAULT_WARNING_FLAGS["strict_warnings"]), | ||||||
| "treat_warnings_as_errors": list(DEFAULT_WARNING_FLAGS["treat_warnings_as_errors"]), | ||||||
| } | ||||||
|
|
||||||
| for mod in mctx.modules: | ||||||
| for tag in mod.tags.toolchain: | ||||||
|
|
@@ -36,14 +103,29 @@ def _gcc_impl(mctx): | |||||
|
|
||||||
| for tag in mod.tags.extra_features: | ||||||
| for feature in tag.features: | ||||||
| features.append(feature) | ||||||
| f = feature.strip() | ||||||
| if not f: | ||||||
| continue | ||||||
| if f.startswith("-"): | ||||||
| remove_feature = f[1:].strip() | ||||||
| if remove_feature in features: | ||||||
| features.remove(remove_feature) | ||||||
| else: | ||||||
| if f not in features: | ||||||
| features.append(f) | ||||||
|
|
||||||
| for tag in mod.tags.warning_flags: | ||||||
| warning_flags = { | ||||||
| "minimal_warnings":tag.minimal_warnings, | ||||||
| "strict_warnings":tag.strict_warnings, | ||||||
| "treat_warnings_as_errors":tag.treat_warnings_as_errors | ||||||
| } | ||||||
| for flag in tag.minimal_warnings: | ||||||
| if flag not in warning_flags["minimal_warnings"]: | ||||||
| warning_flags["minimal_warnings"].append(flag) | ||||||
|
|
||||||
| for flag in tag.strict_warnings: | ||||||
| if flag not in warning_flags["strict_warnings"]: | ||||||
| warning_flags["strict_warnings"].append(flag) | ||||||
|
|
||||||
| for flag in tag.treat_warnings_as_errors: | ||||||
| if flag not in warning_flags["treat_warnings_as_errors"]: | ||||||
| warning_flags["treat_warnings_as_errors"].append(flag) | ||||||
|
|
||||||
| if toolchain_info: | ||||||
| http_archive( | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 8.1.1 | ||
| 8.3.0 |
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.
I'm not sure, but is it like this?
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.
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)