Browse Source

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
Chih-Hung Hsieh 1 year ago
parent
commit
794b81d9d1
3 changed files with 151 additions and 50 deletions
  1. 48 24
      cc/config/tidy.go
  2. 27 26
      cc/tidy.go
  3. 76 0
      cc/tidy_test.go

+ 48 - 24
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 {

+ 27 - 26
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

+ 76 - 0
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