Merge "Prepare to obsolete -warnings-as-errors in tidy_flags" (6ba1a7d6) · Commits · e / os / android_build_soong

cc/config/tidy.go

+48 −24

Original line number	Diff line number	Diff line
		@@ -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 {

cc/tidy.go

+27 −26

Original line number	Diff line number	Diff line
		@@ -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
		// 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=") {
		// 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=-*"
		if NoWarningsAsErrorsInTidyFlags {
		ctx.PropertyErrorf("tidy_flags", "should not contain "+s+"; use tidy_checks_as_errors instead.")
		} else {
		flags.TidyFlags[i] = newFlag + " -warnings-as-errors=-*"
		}
		inserted = true
		break
		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

cc/tidy_test.go

+76 −0

Original line number	Diff line number	Diff line
		@@ -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