diff --git a/cc/tidy.go b/cc/tidy.go index ff49c64bf..e8e17831c 100644 --- a/cc/tidy.go +++ b/cc/tidy.go @@ -188,31 +188,14 @@ func (tidy *tidyFeature) flags(ctx ModuleContext, flags Flags) Flags { tidyChecks = tidyChecks + ",-cert-err33-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 _, s := range flags.TidyFlags { + if strings.Contains(s, "-warnings-as-errors=") { + ctx.PropertyErrorf("tidy_flags", "should not contain -warnings-as-errors, use tidy_checks_as_errors instead") } - if !inserted { - flags.TidyFlags = append(flags.TidyFlags, "-warnings-as-errors=-*") - } - } else if len(tidy.Properties.Tidy_checks_as_errors) > 0 { + } + 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), ",") flags.TidyFlags = append(flags.TidyFlags, tidyChecksAsErrors) } diff --git a/cc/tidy_test.go b/cc/tidy_test.go index 14b33b239..5863a6c17 100644 --- a/cc/tidy_test.go +++ b/cc/tidy_test.go @@ -22,6 +22,76 @@ import ( "android/soong/android" ) +func TestTidyFlagsWarningsAsErrors(t *testing.T) { + // The "tidy_flags" property should not contain -warnings-as-errors. + testCases := []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 + }{ + { + "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"], + tidy_flags: ["-header-filter=dir2/"], + }`, + "", + []string{"-header-filter=dir2/", "-warnings-as-errors='xyz-*',abc"}, + []string{}, + }, + { + "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 -warnings-as-errors,` + + ` 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 for %s does not contain %s.", test.libName, flag) + } + } + for _, flag := range test.noFlags { + if strings.Contains(flags, flag) { + t.Errorf("tidyFlags for %s should not contain %s.", 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