Prepare to obsolete -warnings-as-errors in tidy_flags
* A follow up CL will set const NoWarningsAsErrorsInTidyFlags
and make it an error to use -warnings-as-errors in tidy_flags.
* Append TidyGlobalNoChecks after local tidy_checks to disable some checks.
* Append TidyGlobalNoErrorChecks after local tidy_checks_as_errors
(-warnings-as-errors) to allow some warnings globally.
* Move bugprone-signed-char-misuse and bugprone-branch-clone to
globalNoErrorCheckList so we can find and fix some of those warnings.
Bug: 229801437
Test: WITH_TIDY=1 make; make tidy-soong_subset
Change-Id: I0128b859b7be6eb9bbda1916b89a6a471b052150
diff --git a/cc/config/tidy.go b/cc/config/tidy.go
index f96f3ed..c893320 100644
--- a/cc/config/tidy.go
+++ b/cc/config/tidy.go
@@ -19,6 +19,37 @@
"strings"
)
+var (
+ // Some clang-tidy checks have bugs or not work for Android.
+ // They are disabled here, overriding any locally selected checks.
+ globalNoCheckList = []string{
+ // https://b.corp.google.com/issues/153464409
+ // many local projects enable cert-* checks, which
+ // trigger bugprone-reserved-identifier.
+ "-bugprone-reserved-identifier*,-cert-dcl51-cpp,-cert-dcl37-c",
+ // http://b/153757728
+ "-readability-qualified-auto",
+ // http://b/193716442
+ "-bugprone-implicit-widening-of-multiplication-result",
+ // Too many existing functions trigger this rule, and fixing it requires large code
+ // refactoring. The cost of maintaining this tidy rule outweighs the benefit it brings.
+ "-bugprone-easily-swappable-parameters",
+ // http://b/216364337 - TODO: Follow-up after compiler update to
+ // disable or fix individual instances.
+ "-cert-err33-c",
+ }
+
+ // Some clang-tidy checks are included in some tidy_checks_as_errors lists,
+ // but not all warnings are fixed/suppressed yet. These checks are not
+ // disabled in the TidyGlobalNoChecks list, so we can see them and fix/suppress them.
+ globalNoErrorCheckList = []string{
+ // http://b/155034563
+ "-bugprone-signed-char-misuse",
+ // http://b/155034972
+ "-bugprone-branch-clone",
+ }
+)
+
func init() {
// Many clang-tidy checks like altera-*, llvm-*, modernize-*
// are not designed for Android source code or creating too
@@ -94,29 +125,12 @@
}, ",")
})
- // Some clang-tidy checks have bugs or not work for Android.
- // They are disabled here, overriding any locally selected checks.
pctx.VariableFunc("TidyGlobalNoChecks", func(ctx android.PackageVarContext) string {
- return strings.Join([]string{
- // https://b.corp.google.com/issues/153464409
- // many local projects enable cert-* checks, which
- // trigger bugprone-reserved-identifier.
- "-bugprone-reserved-identifier*,-cert-dcl51-cpp,-cert-dcl37-c",
- // http://b/153757728
- "-readability-qualified-auto",
- // http://b/155034563
- "-bugprone-signed-char-misuse",
- // http://b/155034972
- "-bugprone-branch-clone",
- // http://b/193716442
- "-bugprone-implicit-widening-of-multiplication-result",
- // Too many existing functions trigger this rule, and fixing it requires large code
- // refactoring. The cost of maintaining this tidy rule outweighs the benefit it brings.
- "-bugprone-easily-swappable-parameters",
- // http://b/216364337 - TODO: Follow-up after compiler update to
- // disable or fix individual instances.
- "-cert-err33-c",
- }, ",")
+ return strings.Join(globalNoCheckList, ",")
+ })
+
+ pctx.VariableFunc("TidyGlobalNoErrorChecks", func(ctx android.PackageVarContext) string {
+ return strings.Join(globalNoErrorCheckList, ",")
})
// To reduce duplicate warnings from the same header files,
@@ -140,7 +154,6 @@
const tidyDefault = "${config.TidyDefaultGlobalChecks}"
const tidyExternalVendor = "${config.TidyExternalVendorChecks}"
const tidyDefaultNoAnalyzer = "${config.TidyDefaultGlobalChecks},-clang-analyzer-*"
-const tidyGlobalNoChecks = "${config.TidyGlobalNoChecks}"
// This is a map of local path prefixes to the set of default clang-tidy checks
// to be used.
@@ -180,7 +193,18 @@
// Returns a globally disabled tidy checks, overriding locally selected checks.
func TidyGlobalNoChecks() string {
- return tidyGlobalNoChecks
+ if len(globalNoCheckList) > 0 {
+ return ",${config.TidyGlobalNoChecks}"
+ }
+ return ""
+}
+
+// Returns a globally allowed/no-error tidy checks, appended to -warnings-as-errors.
+func TidyGlobalNoErrorChecks() string {
+ if len(globalNoErrorCheckList) > 0 {
+ return ",${config.TidyGlobalNoErrorChecks}"
+ }
+ return ""
}
func TidyFlagsForSrcFile(srcFile android.Path, flags string) string {
diff --git a/cc/tidy.go b/cc/tidy.go
index 75c038d..94b10c2 100644
--- a/cc/tidy.go
+++ b/cc/tidy.go
@@ -15,6 +15,7 @@
package cc
import (
+ "fmt"
"path/filepath"
"regexp"
"strings"
@@ -62,6 +63,11 @@
return []interface{}{&tidy.Properties}
}
+// Set this const to true when all -warnings-as-errors in tidy_flags
+// are replaced with tidy_checks_as_errors.
+// Then, that old style usage will be obsolete and an error.
+const NoWarningsAsErrorsInTidyFlags = false
+
func (tidy *tidyFeature) flags(ctx ModuleContext, flags Flags) Flags {
CheckBadTidyFlags(ctx, "tidy_flags", tidy.Properties.Tidy_flags)
CheckBadTidyChecks(ctx, "tidy_checks", tidy.Properties.Tidy_checks)
@@ -161,42 +167,37 @@
config.ClangRewriteTidyChecks(localChecks)), ",")
}
}
- tidyChecks = tidyChecks + "," + config.TidyGlobalNoChecks()
+ tidyChecks = tidyChecks + config.TidyGlobalNoChecks()
if ctx.Windows() {
// https://b.corp.google.com/issues/120614316
// mingw32 has cert-dcl16-c warning in NO_ERROR,
// which is used in many Android files.
- tidyChecks = tidyChecks + ",-cert-dcl16-c"
+ tidyChecks += ",-cert-dcl16-c"
}
flags.TidyFlags = append(flags.TidyFlags, tidyChecks)
- if ctx.Config().IsEnvTrue("WITH_TIDY") {
- // WITH_TIDY=1 enables clang-tidy globally. There could be many unexpected
- // warnings from new checks and many local tidy_checks_as_errors and
- // -warnings-as-errors can break a global build.
- // So allow all clang-tidy warnings.
- inserted := false
- for i, s := range flags.TidyFlags {
- if strings.Contains(s, "-warnings-as-errors=") {
- // clang-tidy accepts only one -warnings-as-errors
- // replace the old one
- re := regexp.MustCompile(`'?-?-warnings-as-errors=[^ ]* *`)
- newFlag := re.ReplaceAllString(s, "")
- if newFlag == "" {
- flags.TidyFlags[i] = "-warnings-as-errors=-*"
- } else {
- flags.TidyFlags[i] = newFlag + " -warnings-as-errors=-*"
- }
- inserted = true
- break
+ // Embedding -warnings-as-errors in tidy_flags is error-prone.
+ // It should be replaced with the tidy_checks_as_errors list.
+ for i, s := range flags.TidyFlags {
+ if strings.Contains(s, "-warnings-as-errors=") {
+ if NoWarningsAsErrorsInTidyFlags {
+ ctx.PropertyErrorf("tidy_flags", "should not contain "+s+"; use tidy_checks_as_errors instead.")
+ } else {
+ fmt.Printf("%s: warning: module %s's tidy_flags should not contain %s, which is replaced with -warnings-as-errors=-*; use tidy_checks_as_errors for your own as-error warnings instead.\n",
+ ctx.BlueprintsFile(), ctx.ModuleName(), s)
+ flags.TidyFlags[i] = "-warnings-as-errors=-*"
}
+ break // there is at most one -warnings-as-errors
}
- if !inserted {
- flags.TidyFlags = append(flags.TidyFlags, "-warnings-as-errors=-*")
- }
- } else if len(tidy.Properties.Tidy_checks_as_errors) > 0 {
- tidyChecksAsErrors := "-warnings-as-errors=" + strings.Join(esc(ctx, "tidy_checks_as_errors", tidy.Properties.Tidy_checks_as_errors), ",")
+ }
+ // Default clang-tidy flags does not contain -warning-as-errors.
+ // If a module has tidy_checks_as_errors, add the list to -warnings-as-errors
+ // and then append the TidyGlobalNoErrorChecks.
+ if len(tidy.Properties.Tidy_checks_as_errors) > 0 {
+ tidyChecksAsErrors := "-warnings-as-errors=" +
+ strings.Join(esc(ctx, "tidy_checks_as_errors", tidy.Properties.Tidy_checks_as_errors), ",") +
+ config.TidyGlobalNoErrorChecks()
flags.TidyFlags = append(flags.TidyFlags, tidyChecksAsErrors)
}
return flags
diff --git a/cc/tidy_test.go b/cc/tidy_test.go
index 5c15fac..7036ecb 100644
--- a/cc/tidy_test.go
+++ b/cc/tidy_test.go
@@ -22,6 +22,82 @@
"android/soong/android"
)
+func TestTidyFlagsWarningsAsErrors(t *testing.T) {
+ // The "tidy_flags" property should not contain -warnings-as-errors.
+ type testCase struct {
+ libName, bp string
+ errorMsg string // a negative test; must have error message
+ flags []string // must have substrings in tidyFlags
+ noFlags []string // must not have substrings in tidyFlags
+ }
+
+ testCases := []testCase{
+ {
+ "libfoo1",
+ `cc_library_shared { // no warnings-as-errors, good tidy_flags
+ name: "libfoo1",
+ srcs: ["foo.c"],
+ tidy_flags: ["-header-filter=dir1/"],
+ }`,
+ "",
+ []string{"-header-filter=dir1/"},
+ []string{"-warnings-as-errors"},
+ },
+ {
+ "libfoo2",
+ `cc_library_shared { // good use of tidy_checks_as_errors
+ name: "libfoo2",
+ srcs: ["foo.c"],
+ tidy_checks_as_errors: ["xyz-*", "abc"],
+ }`,
+ "",
+ []string{
+ "-header-filter=^", // there is a default header filter
+ "-warnings-as-errors='xyz-*',abc,${config.TidyGlobalNoErrorChecks}",
+ },
+ []string{},
+ },
+ }
+ if NoWarningsAsErrorsInTidyFlags {
+ testCases = append(testCases, testCase{
+ "libfoo3",
+ `cc_library_shared { // bad use of -warnings-as-errors in tidy_flags
+ name: "libfoo3",
+ srcs: ["foo.c"],
+ tidy_flags: [
+ "-header-filters=.*",
+ "-warnings-as-errors=xyz-*",
+ ],
+ }`,
+ `module "libfoo3" .*: tidy_flags: should not contain .*;` +
+ ` use tidy_checks_as_errors instead`,
+ []string{},
+ []string{},
+ })
+ }
+ for _, test := range testCases {
+ if test.errorMsg != "" {
+ testCcError(t, test.errorMsg, test.bp)
+ continue
+ }
+ variant := "android_arm64_armv8-a_shared"
+ ctx := testCc(t, test.bp)
+ t.Run("caseTidyFlags", func(t *testing.T) {
+ flags := ctx.ModuleForTests(test.libName, variant).Rule("clangTidy").Args["tidyFlags"]
+ for _, flag := range test.flags {
+ if !strings.Contains(flags, flag) {
+ t.Errorf("tidyFlags %v for %s does not contain %s.", flags, test.libName, flag)
+ }
+ }
+ for _, flag := range test.noFlags {
+ if strings.Contains(flags, flag) {
+ t.Errorf("tidyFlags %v for %s should not contain %s.", flags, test.libName, flag)
+ }
+ }
+ })
+ }
+}
+
func TestTidyChecks(t *testing.T) {
// The "tidy_checks" property defines additional checks appended
// to global default. But there are some checks disabled after