Disallow -warnings-as-errors in tidy_flags
* Also remove the undocumented complicated
experiment to overwrite local warnings-as-errors.
Bug: 229801437
Test: WITH_TIDY=1 make; make tidy-soong_subset
Change-Id: I2fb32146b4685ab9f5198724c15c303f799b7a14
diff --git a/cc/tidy.go b/cc/tidy.go
index ff49c64..e8e1783 100644
--- a/cc/tidy.go
+++ b/cc/tidy.go
@@ -188,31 +188,14 @@
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 14b33b2..5863a6c 100644
--- a/cc/tidy_test.go
+++ b/cc/tidy_test.go
@@ -22,6 +22,76 @@
"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