diff --git a/cc/config/tidy.go b/cc/config/tidy.go index f96f3ed41..c89332067 100644 --- a/cc/config/tidy.go +++ b/cc/config/tidy.go @@ -19,6 +19,37 @@ import ( "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 @@ func init() { }, ",") }) - // 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 @@ type PathBasedTidyCheck struct { 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 @@ func TidyChecksForDir(dir string) string { // 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 75c038d5e..94b10c2d6 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 @@ func (tidy *tidyFeature) props() []interface{} { 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 @@ func (tidy *tidyFeature) flags(ctx ModuleContext, flags Flags) Flags { 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 5c15fac00..7036ecb1a 100644 --- a/cc/tidy_test.go +++ b/cc/tidy_test.go @@ -22,6 +22,82 @@ import ( "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