Merge "Add tidy.md document"
This commit is contained in:
Chih-hung Hsieh
Gerrit Code Review
commit
7892c6d76d
2 changed files with 245 additions and 0 deletions
|
|
@ -1,2 +1,3 @@
|
||||||
per-file map_files.md = danalbert@google.com, enh@google.com, jiyong@google.com
|
per-file map_files.md = danalbert@google.com, enh@google.com, jiyong@google.com
|
||||||
per-file native_code_coverage.md = pirama@google.com, srhines@google.com, allenhair@google.com
|
per-file native_code_coverage.md = pirama@google.com, srhines@google.com, allenhair@google.com
|
||||||
|
per-file tidy.md = chh@google.com, srhines@google.com
|
||||||
|
|
|
||||||
244
docs/tidy.md
Normal file
244
docs/tidy.md
Normal file
|
|
@ -0,0 +1,244 @@
|
||||||
|
# Clang-Tidy Rules and Checks
|
||||||
|
|
||||||
|
Android C/C++ source files can be checked by clang-tidy for issues like
|
||||||
|
coding style, error-prone/performance patterns, and static flow analysis.
|
||||||
|
See the official
|
||||||
|
[clang-tidy document](https://clang.llvm.org/extra/clang-tidy)
|
||||||
|
and list of
|
||||||
|
[clang-tidy checks](https://clang.llvm.org/extra/clang-tidy/checks/list.html).
|
||||||
|
|
||||||
|
## Global defaults for Android builds
|
||||||
|
|
||||||
|
The simplest way to enable clang-tidy checks is
|
||||||
|
to set environment variable `WITH_TIDY`.
|
||||||
|
```
|
||||||
|
$ WITH_TIDY=1 make
|
||||||
|
```
|
||||||
|
|
||||||
|
This will turn on the global default to run clang-tidy for every required
|
||||||
|
C/C++ source file compilation. The global default clang-tidy checks
|
||||||
|
do not include time-consuming static analyzer checks. To enable those
|
||||||
|
checks, set the `CLANG_ANALYZER_CHECKS` variable.
|
||||||
|
```
|
||||||
|
$ WITH_TIDY=1 CLANG_ANALYZER_CHECKS=1 make
|
||||||
|
```
|
||||||
|
|
||||||
|
The default global clang-tidy checks and flags are defined in
|
||||||
|
[build/soong/cc/config/tidy.go](https://android.googlesource.com/platform/build/soong/+/refs/heads/master/cc/config/tidy.go).
|
||||||
|
|
||||||
|
|
||||||
|
## Module clang-tidy properties
|
||||||
|
|
||||||
|
The global default can be overwritten by module properties in Android.bp.
|
||||||
|
|
||||||
|
### `tidy` and `tidy_checks`
|
||||||
|
|
||||||
|
For example, in
|
||||||
|
[system/bpf/Android.bp](https://android.googlesource.com/platform/system/bpf/+/refs/heads/master/Android.bp),
|
||||||
|
clang-tidy is enabled explicitly and with a different check list:
|
||||||
|
```
|
||||||
|
cc_defaults {
|
||||||
|
name: "bpf_defaults",
|
||||||
|
// snipped
|
||||||
|
tidy: true,
|
||||||
|
tidy_checks: [
|
||||||
|
"android-*",
|
||||||
|
"cert-*",
|
||||||
|
"-cert-err34-c",
|
||||||
|
"clang-analyzer-security*",
|
||||||
|
// Disabling due to many unavoidable warnings from POSIX API usage.
|
||||||
|
"-google-runtime-int",
|
||||||
|
],
|
||||||
|
}
|
||||||
|
```
|
||||||
|
That means in normal builds, even without `WITH_TIDY=1`,
|
||||||
|
the modules that use `bpf_defaults` will run clang-tidy
|
||||||
|
over C/C++ source files with the given `tidy_checks`.
|
||||||
|
Note that `clang-analyzer-security*` is included in `tidy_checks`
|
||||||
|
but not all `clang-analyzer-*` checks. Check `cert-err34-c` is
|
||||||
|
disabled, although `cert-*` is selected.
|
||||||
|
|
||||||
|
Some modules might want to disable clang-tidy even when
|
||||||
|
environment variable `WITH_TIDY=1` is set.
|
||||||
|
Examples can be found in
|
||||||
|
[system/netd/tests/Android.bp](https://android.googlesource.com/platform/system/netd/+/refs/heads/master/tests/Android.bp)
|
||||||
|
```
|
||||||
|
cc_test {
|
||||||
|
name: "netd_integration_test",
|
||||||
|
// snipped
|
||||||
|
defaults: ["netd_defaults"],
|
||||||
|
tidy: false, // cuts test build time by almost 1 minute
|
||||||
|
```
|
||||||
|
and in
|
||||||
|
[bionic/tests/Android.bp](https://android.googlesource.com/platform/bionic/+/refs/heads/master/tests/Android.bp).
|
||||||
|
```
|
||||||
|
cc_test_library {
|
||||||
|
name: "fortify_disabled_for_tidy",
|
||||||
|
// snipped
|
||||||
|
srcs: ["clang_fortify_tests.cpp"],
|
||||||
|
tidy: false,
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
### `tidy_checks_as_errors`
|
||||||
|
|
||||||
|
The global tidy checks are enabled as warnings.
|
||||||
|
If a C/C++ module wants to be free of certain clang-tidy warnings,
|
||||||
|
it can chose those checks to be treated as errors.
|
||||||
|
For example
|
||||||
|
[system/core/libsysutils/Android.bp](https://android.googlesource.com/platform/system/core/+/refs/heads/master/libsysutils/Android.bp)
|
||||||
|
has enabled clang-tidy explicitly, selected its own tidy checks,
|
||||||
|
and set three groups of tidy checks as errors:
|
||||||
|
```
|
||||||
|
cc_library {
|
||||||
|
name: "libsysutils",
|
||||||
|
// snipped
|
||||||
|
tidy: true,
|
||||||
|
tidy_checks: [
|
||||||
|
"-*",
|
||||||
|
"cert-*",
|
||||||
|
"clang-analyzer-security*",
|
||||||
|
"android-*",
|
||||||
|
],
|
||||||
|
tidy_checks_as_errors: [
|
||||||
|
"cert-*",
|
||||||
|
"clang-analyzer-security*",
|
||||||
|
"android-*",
|
||||||
|
],
|
||||||
|
// snipped
|
||||||
|
}
|
||||||
|
```
|
||||||
|
|
||||||
|
### `tidy_flags` and `tidy_disabled_srcs`
|
||||||
|
|
||||||
|
Extra clang-tidy flags can be passed with the `tidy_flags` property.
|
||||||
|
|
||||||
|
Some Android modules use the `tidy_flags` to pass "-warnings-as-errors="
|
||||||
|
to clang-tidy. This usage should now be replaced with the
|
||||||
|
`tidy_checks_as_errors` property.
|
||||||
|
|
||||||
|
Some other tidy flags examples are `-format-style=` and `-header-filter=`
|
||||||
|
For example, in
|
||||||
|
[art/odrefresh/Android.bp](https://android.googlesource.com/platform/art/+/refs/heads/master/odrefresh/Android.bp),
|
||||||
|
we found
|
||||||
|
```
|
||||||
|
cc_defaults {
|
||||||
|
name: "odrefresh-defaults",
|
||||||
|
srcs: [
|
||||||
|
"odrefresh.cc",
|
||||||
|
"odr_common.cc",
|
||||||
|
"odr_compilation_log.cc",
|
||||||
|
"odr_fs_utils.cc",
|
||||||
|
"odr_metrics.cc",
|
||||||
|
"odr_metrics_record.cc",
|
||||||
|
],
|
||||||
|
// snipped
|
||||||
|
generated_sources: [
|
||||||
|
"apex-info-list-tinyxml",
|
||||||
|
"art-apex-cache-info",
|
||||||
|
"art-odrefresh-operator-srcs",
|
||||||
|
],
|
||||||
|
// snipped
|
||||||
|
tidy: true,
|
||||||
|
tidy_disabled_srcs: [":art-apex-cache-info"],
|
||||||
|
tidy_flags: [
|
||||||
|
"-format-style=file",
|
||||||
|
"-header-filter=(art/odrefresh/|system/apex/)",
|
||||||
|
],
|
||||||
|
}
|
||||||
|
```
|
||||||
|
That means all modules with the `odrefresh-defaults` will
|
||||||
|
have clang-tidy enabled, but not for generated source
|
||||||
|
files in `art-apex-cache-info`.
|
||||||
|
The clang-tidy is called with extra flags to specify the
|
||||||
|
format-style and header-filter.
|
||||||
|
|
||||||
|
Note that the globally set default for header-filter is to
|
||||||
|
include only the module directory. So, the default clang-tidy
|
||||||
|
warnings for `art/odrefresh` modules will include source files
|
||||||
|
under that directory. Now `odrefresh-defaults` is interested
|
||||||
|
in seeing warnings from both `art/odrefresh/` and `system/apex/`
|
||||||
|
and it redefines `-header-filter` in its `tidy_flags`.
|
||||||
|
|
||||||
|
|
||||||
|
## Phony tidy-* targets
|
||||||
|
|
||||||
|
### The tidy-*directory* targets
|
||||||
|
|
||||||
|
Setting `WITH_TIDY=1` is easy to enable clang-tidy globally for any build.
|
||||||
|
However, it adds extra compilation time.
|
||||||
|
|
||||||
|
For developers focusing on just one directory, they only want to compile
|
||||||
|
their files with clang-tidy and wish to build other Android components as
|
||||||
|
fast as possible. Changing the `WITH_TIDY=1` variable setting is also expensive
|
||||||
|
since the build.ninja file will be regenerated due to any such variable change.
|
||||||
|
|
||||||
|
To manually select only some directories or modules to compile with clang-tidy,
|
||||||
|
do not set the `WITH_TIDY=1` variable, but use the special `tidy-<directory>`
|
||||||
|
phony target. For example, a person working on `system/libbase` can build
|
||||||
|
Android quickly with
|
||||||
|
```
|
||||||
|
unset WITH_TIDY # Optional, not if you haven't set WITH_TIDY
|
||||||
|
make droid tidy-system-libbase
|
||||||
|
```
|
||||||
|
|
||||||
|
For any directory `d1/d2/d3`, a phony target tidy-d1-d2-d3 is generated
|
||||||
|
if there is any C/C++ source file under `d1/d2/d3`.
|
||||||
|
|
||||||
|
Note that with `make droid tidy-system-libbase`, some C/C++ files
|
||||||
|
that are not needed by the `droid` target will be passed to clang-tidy
|
||||||
|
if they are under `system/libbase`. This is like a `checkbuild`
|
||||||
|
under `system/libbase` to include all modules, but only C/C++
|
||||||
|
files of those modules are compiled with clang-tidy.
|
||||||
|
|
||||||
|
### The tidy-soong target
|
||||||
|
|
||||||
|
A special `tidy-soong` target is defined to include all C/C++
|
||||||
|
source files in *all* directories. This phony target is sometimes
|
||||||
|
used to test if all source files compile with a new clang-tidy release.
|
||||||
|
|
||||||
|
### The tidy-*_subset targets
|
||||||
|
|
||||||
|
A *subset* of each tidy-* phony target is defined to reduce test time.
|
||||||
|
Since any Android module, a C/C++ library or binary, can be built
|
||||||
|
for many different *variants*, one C/C++ source file is usually
|
||||||
|
compiled multiple times with different compilation flags.
|
||||||
|
Many of such *variant* flags have little or no effect on clang-tidy
|
||||||
|
checks. To reduce clang-tidy check time, a *subset* target like
|
||||||
|
`tidy-soong_subset` or `tidy-system-libbase_subset` is generated
|
||||||
|
to include only a subset, the first variant, of each module in
|
||||||
|
the directory.
|
||||||
|
|
||||||
|
Hence, for C/C++ source code quality, instead of a long
|
||||||
|
"make checkbuild", we can use "make tidy-soong_subset".
|
||||||
|
|
||||||
|
|
||||||
|
## Limit clang-tidy runtime
|
||||||
|
|
||||||
|
Some Android modules have large files that take a long time to compile
|
||||||
|
with clang-tidy, with or without the clang-analyzer checks.
|
||||||
|
To limit clang-tidy time, an environment variable can be set as
|
||||||
|
```base
|
||||||
|
WITH_TIDY=1 TIDY_TIMEOUT=90 make
|
||||||
|
```
|
||||||
|
This 90-second limit is actually the default time limit
|
||||||
|
in several Android continuous builds where `WITH_TIDY=1` and
|
||||||
|
`CLANG_ANALYZER_CHECKS=1` are set.
|
||||||
|
|
||||||
|
## Capabilities for Android.bp and Android.mk
|
||||||
|
|
||||||
|
Some of the previously mentioned features are defined only
|
||||||
|
for modules in Android.bp files, not for Android.mk modules yet.
|
||||||
|
|
||||||
|
* The global `WITH_TIDY=1` variable will enable clang-tidy for all C/C++
|
||||||
|
modules in Android.bp or Android.mk files.
|
||||||
|
|
||||||
|
* The global `TIDY_TIMEOUT` variable is recognized by Android prebuilt
|
||||||
|
clang-tidy, so it should work for any clang-tidy invocation.
|
||||||
|
|
||||||
|
* The clang-tidy module level properties are defined for Android.bp modules.
|
||||||
|
For Android.mk modules, old `LOCAL_TIDY`, `LOCAL_TIDY_CHECKS`,
|
||||||
|
`LOCAL_TIDY_FLAGS` work similarly, but it would be better to convert
|
||||||
|
those modules to use Android.bp files.
|
||||||
|
|
||||||
|
* The `tidy-*` phony targets are only generated for Android.bp modules.
|
||||||
Loading…
Reference in a new issue