Code quality day: Clang-Tidy #78535

New Issue

Sergey Sharybin · 2020-07-02T15:38:35+02:00

Sergey Sharybin commented

2020-07-02 15:38:35 +02:00

The goal of this little project is to allow developers to leverage Clang-Tidy to help spotting typical bugprone code and typical readability problems. The plan of attack is simple: a developer picks up a warning from the list, modifies the code base to address it, commits the work to the master Git branch.

Required setup:

Linux (unfortunately, it is not trivial to have Clang-Tidy outside of Linux)
Clang-Tidy installed (use package manager of your Linux distro)
Configure Blender's CMake using WITH_CLANG_TIDY=ON (optionally specify CLANG_TIDY_EXECUTABLE if automatic detection fails).

Both Clang and GCC toolchains are supposed to work. However, GCC+Clang-Tidy might need some polishing.

There is a command which works for me (tm): make developer debug BUILD_CMAKE_ARGS='-DWITH_CLANG_TIDY=ON'

Clang-Tidy is run as an extra compilation step, so it's warning are emitted as regular build process.

Workflow

Put your name next to the warning once you start working on it.
Remove corresponding line from the .clang-tidy file, make Blender compiled without this warning.
Commit result of your work (if the warning is fully tackled, commit the change to .clang-tidy as well).

Some warnings happens in many areas, and many people can work on it. Use blender-chat to coordinate this.

List of warnings to be addressed

Documentation can be found in the Clang-Tidy v10 docs.

NOTE: This list is assembled based on thoughts "would be really nice to address those". Use common sense when it seems that addressing them goes too far. It might be best to ignore the warning rather than heavily modify codebase at this point.

NOTE: Some of the cleanup might discover actual bug in the code. Please separate fixing the bug from the cleanup commit.

-readability-else-after-return
-readability-braces-around-statements
-readability-inconsistent-declaration-parameter-name
-readability-non-const-parameter
-readability-redundant-preprocessor
-readability-redundant-control-flow
-readability-named-parameter
-readability-function-size (rule is enabled, but future parameter tweaks will likely reveal more areas of improvement)
-readability-static-definition-in-anonymous-namespace
-readability-delete-null-pointer
-readability-redundant-string-init
-readability-redundant-member-init
-readability-const-return-type
-readability-container-size-empty
-readability-redundant-string-cstr
-readability-static-accessed-through-instance
-readability-redundant-declaration
-readability-qualified-auto
-readability-function-cognitive-complexity
-readability-suspicious-call-argument
-bugprone-incorrect-roundings
-bugprone-suspicious-string-compare
-bugprone-too-small-loop-variable
-bugprone-misplaced-widening-cast
-bugprone-not-null-terminated-result
-bugprone-suspicious-missing-comma
-bugprone-argument-comment
-bugprone-assert-side-effect
-bugprone-parent-virtual-call
-bugprone-infinite-loop
-bugprone-copy-constructor-init
-bugprone-lambda-function-name
-bugprone-exception-escape (does not happen on Linux + clang-tidy-10) (Fixed with D9497)
-bugprone-redundant-branch-condition
-bugprone-easily-swappable-parameters
-bugprone-implicit-widening-of-multiplication-result
-modernize-deprecated-headers
-modernize-use-equals-default D9494
-modernize-use-nullptr
-modernize-concat-nested-namespaces
-modernize-use-emplace
-modernize-use-using
-modernize-redundant-void-arg
-modernize-use-bool-literals
-modernize-loop-convert (partially done in 11c4066159)
-modernize-pass-by-value (P1755)
-modernize-make-unique
-modernize-use-override
-modernize-use-transparent-functors D10323
-modernize-raw-string-literal D10322
-modernize-avoid-bind D10320
-modernize-use-default-member-init D9501
-modernize-return-braced-init-list

There is -readability-misleading-indentation explicitly silenced. Seems to be incompatible with Blender's clang-format rules.

Ignored for now:

-bugprone-sizeof-expression: It does not allow us to do the following: T **ptr_array = MEM_mallocN(sizeof(*ptr_array), AT). Also see this .
-bugprone-integer-division: It causes too many false-positives and fixing adds more noise.

Papercuts

] Modifying .clang-tidy does not force re-compilation. For now run the clean target manually to make sure new warnings are enabled. This is something what was annoying initially, but is not that different from .clang-format. Lets not overthink/overcomplicate build process here and just leave it as is.

The goal of this little project is to allow developers to leverage Clang-Tidy to help spotting typical bugprone code and typical readability problems. The plan of attack is simple: a developer picks up a warning from the list, modifies the code base to address it, commits the work to the master Git branch. Required setup: * Linux (unfortunately, it is not trivial to have Clang-Tidy outside of Linux) * Clang-Tidy installed (use package manager of your Linux distro) * Configure Blender's CMake using `WITH_CLANG_TIDY=ON` (optionally specify `CLANG_TIDY_EXECUTABLE` if automatic detection fails). Both Clang and GCC toolchains are supposed to work. However, GCC+Clang-Tidy might need some polishing. There is a command which `works for me (tm)`: `make developer debug BUILD_CMAKE_ARGS='-DWITH_CLANG_TIDY=ON'` Clang-Tidy is run as an extra compilation step, so it's warning are emitted as regular build process. **Workflow** 1. Put your name next to the warning once you start working on it. 2. Remove corresponding line from the `.clang-tidy` file, make Blender compiled without this warning. 3. Commit result of your work (if the warning is fully tackled, commit the change to `.clang-tidy` as well). Some warnings happens in many areas, and many people can work on it. Use blender-chat to coordinate this. **List of warnings to be addressed** Documentation can be found in the [Clang-Tidy v10 docs](https://releases.llvm.org/10.0.0/tools/clang/tools/extra/docs/clang-tidy/checks/list.html). NOTE: This list is assembled based on thoughts "would be really nice to address those". Use common sense when it seems that addressing them goes too far. It might be best to ignore the warning rather than heavily modify codebase at this point. NOTE: Some of the cleanup might discover actual bug in the code. Please separate fixing the bug from the cleanup commit. - [x] `-readability-else-after-return` - [x] `-readability-braces-around-statements` - [x] `-readability-inconsistent-declaration-parameter-name` - [x] `-readability-non-const-parameter` - [x] `-readability-redundant-preprocessor` - [x] `-readability-redundant-control-flow` - [x] `-readability-named-parameter` - [x] `-readability-function-size` (rule is enabled, but future parameter tweaks will likely reveal more areas of improvement) - [x] `-readability-static-definition-in-anonymous-namespace` - [x] `-readability-delete-null-pointer` - [x] `-readability-redundant-string-init` - [ ] `-readability-redundant-member-init` - [x] `-readability-const-return-type` - [x] `-readability-container-size-empty` - [x] `-readability-redundant-string-cstr` - [x] `-readability-static-accessed-through-instance` - [x] `-readability-redundant-declaration` - [x] `-readability-qualified-auto` - [ ] `-readability-function-cognitive-complexity` - [ ] `-readability-suspicious-call-argument` - [x] `-bugprone-incorrect-roundings` - [x] `-bugprone-suspicious-string-compare` - [x] `-bugprone-too-small-loop-variable` - [x] `-bugprone-misplaced-widening-cast` - [x] `-bugprone-not-null-terminated-result` - [x] `-bugprone-suspicious-missing-comma` - [x] `-bugprone-argument-comment` - [x] `-bugprone-assert-side-effect` - [x] `-bugprone-parent-virtual-call` - [x] `-bugprone-infinite-loop` - [x] `-bugprone-copy-constructor-init` - [x] `-bugprone-lambda-function-name` - [x] `-bugprone-exception-escape` (does not happen on Linux + clang-tidy-10) (Fixed with [D9497](https://archive.blender.org/developer/D9497)) - [ ] `-bugprone-redundant-branch-condition` - [ ] `-bugprone-easily-swappable-parameters` - [ ] `-bugprone-implicit-widening-of-multiplication-result` - [x] `-modernize-deprecated-headers` - [x] `-modernize-use-equals-default` [D9494](https://archive.blender.org/developer/D9494) - [x] `-modernize-use-nullptr` - [x] `-modernize-concat-nested-namespaces` - [x] `-modernize-use-emplace` - [x] `-modernize-use-using` - [x] `-modernize-redundant-void-arg` - [x] `-modernize-use-bool-literals` - [ ] `-modernize-loop-convert` (partially done in 11c4066159) - [ ] `-modernize-pass-by-value` ([P1755](https://archive.blender.org/developer/P1755.txt)) - [x] `-modernize-make-unique` - [x] `-modernize-use-override` - [x] `-modernize-use-transparent-functors` [D10323](https://archive.blender.org/developer/D10323) - [x] `-modernize-raw-string-literal` [D10322](https://archive.blender.org/developer/D10322) - [x] `-modernize-avoid-bind` [D10320](https://archive.blender.org/developer/D10320) - [x] `-modernize-use-default-member-init` [D9501](https://archive.blender.org/developer/D9501) - [ ] `-modernize-return-braced-init-list` There is `-readability-misleading-indentation` explicitly silenced. Seems to be incompatible with Blender's clang-format rules. Ignored for now: * `-bugprone-sizeof-expression`: It does not allow us to do the following: `T **ptr_array = MEM_mallocN(sizeof(*ptr_array), AT)`. Also see [this ](https://clang.llvm.org/extra/clang-tidy/checks/bugprone-sizeof-expression.html#suspicious-usage-of-sizeof-a). * `-bugprone-integer-division`: It causes too many false-positives and fixing adds more noise. **Papercuts** - ] Modifying .clang-tidy does not force re-compilation. For now run the `clean` target manually to make sure new warnings are enabled. This is something what was annoying initially, but is not that different from .clang-format. Lets not overthink/overcomplicate build process here and just leave it as is.

Sergey Sharybin self-assigned this 2020-07-02 15:38:35 +02:00

Sergey Sharybin commented

2020-07-02 15:38:35 +02:00

Changed status from 'Needs Triage' to: 'Confirmed'

Sergey Sharybin commented

2020-07-02 15:38:35 +02:00

Added subscriber: @Sergey

Sybren A. Stüvel commented

2020-07-03 11:25:16 +02:00

Added subscriber: @dr.sybren

Jacques Lucke commented

2020-07-03 12:36:58 +02:00

Added subscriber: @JacquesLucke

Sebastian Parborg commented

2020-07-03 12:52:55 +02:00

Added subscriber: @ZedDB

Hans Goudey commented

2020-07-03 15:07:13 +02:00

Added subscriber: @HooglyBoogly

Sebastián Barschkis commented

2020-07-03 17:30:46 +02:00

Added subscriber: @sebbas

Ankit Meel commented

2020-10-20 21:48:26 +02:00

Added subscriber: @ankitm

Ankit Meel commented

2020-10-20 21:48:26 +02:00

*Files in makesrna were left out of clang tidy by using it with compiler. Here's what works without actually compiling Blender:

 /usr/share/clang/run-clang-tidy.py  -j 4  -p ../path_to_compilation_database  -fix  files source/

03ded5497a/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py

*There are a few new(?) warnings: bugprone-exception-escape, readability-named-parameter, readability-function-cognitive-complexity, bugprone-redundant-branch-condition, readability-redundant-string-init

Log with readability-function-cognitive-complexity enabled: clang_tidy_log2.log.xz
Others generate very less errors:
clang_tidy_log.log

*Files in `makesrna` were left out of clang tidy by using it with compiler. Here's what works without actually compiling Blender: ``` /usr/share/clang/run-clang-tidy.py -j 4 -p ../path_to_compilation_database -fix files source/ ``` https://github.com/llvm/llvm-project/blob/03ded5497a2f458b6af054fa7bac0da0240e7b7a/clang-tools-extra/clang-tidy/tool/run-clang-tidy.py *There are a few new(?) warnings: `bugprone-exception-escape`, `readability-named-parameter`, `readability-function-cognitive-complexity`, `bugprone-redundant-branch-condition`, `readability-redundant-string-init` Log with `readability-function-cognitive-complexity` enabled: [clang_tidy_log2.log.xz](https://archive.blender.org/developer/F9024100/clang_tidy_log2.log.xz) Others generate very less errors: [clang_tidy_log.log](https://archive.blender.org/developer/F9024187/clang_tidy_log.log)

Sybren A. Stüvel commented

2020-11-06 11:08:24 +01:00

readability-function-cognitive-complexity requires Clang 12, which hasn't even been released yet. I really, really want to enable this rule, but I think we should wait until there is a stable version of Clang that supports it.

`readability-function-cognitive-complexity` requires Clang 12, which hasn't even been released yet. I really, really want to enable this rule, but I think we should wait until there is a stable version of Clang that supports it.

Sergey Sharybin commented

2020-11-06 12:17:43 +01:00

I've expanded the modernize category in 3b77c670f0. Lets find out which of the points we want/plan to address!

Those I find are nice candidates to address:

modernize-deprecated-headers
modernize-use-equals-default
modernize-use-nullptr
modernize-concat-nested-namespaces
modernize-use-emplace
modernize-use-using
modernize-redundant-void-arg
modernize-use-bool-literals
modernize-loop-convert
modernize-pass-by-value
modernize-make-unique
modernize-use-override
modernize-use-transparent-functors

Something which seems interesting to investigate:

modernize-raw-string-literal
modernize-avoid-bind
modernize-use-default-member-init

I've expanded the modernize category in 3b77c670f0. Lets find out which of the points we want/plan to address! Those I find are nice candidates to address: * modernize-deprecated-headers * modernize-use-equals-default * modernize-use-nullptr * modernize-concat-nested-namespaces * modernize-use-emplace * modernize-use-using * modernize-redundant-void-arg * modernize-use-bool-literals * modernize-loop-convert * modernize-pass-by-value * modernize-make-unique * modernize-use-override * modernize-use-transparent-functors Something which seems interesting to investigate: * modernize-raw-string-literal * modernize-avoid-bind * modernize-use-default-member-init

Sybren A. Stüvel commented

2020-11-06 13:02:15 +01:00

Lets find out which of the points we want/plan to address!

All of them! Well, at least the ones you list here seem nice.

modernize-use-trailing-return-type and modernize-use-auto do some things I don't really agree with.

> Lets find out which of the points we want/plan to address! All of them! Well, at least the ones you list here seem nice. [modernize-use-trailing-return-type](https:*clang.llvm.org/extra/clang-tidy/checks/modernize-use-trailing-return-type.html) and [modernize-use-auto](https:*clang.llvm.org/extra/clang-tidy/checks/modernize-use-auto.html) do some things I don't really agree with.

jim man commented

2021-07-17 21:36:04 +02:00

Added subscriber: @jim-man

jim man commented

2021-07-17 21:36:04 +02:00

Clang-Tidy 12 have been released

[Clang-Tidy 12](https://releases.llvm.org/12.0.0/tools/clang/tools/extra/docs/ReleaseNotes.html) have been released

Sergey Sharybin commented

2021-10-06 12:02:54 +02:00

Would be nice to make a final effort and get this task fully done!

@dr.sybren, did you start looking into -readability-function-cognitive-complexity and -bugprone-redundant-branch-condition?

@JacquesLucke, any specific reason why -modernize-loop-convert and -modernize-pass-by-value are only partial and a non-committed patch? Something you didn't have time yet, or something you are not sure it's a good change?

Would be nice to make a final effort and get this task fully done! @dr.sybren, did you start looking into `-readability-function-cognitive-complexity` and `-bugprone-redundant-branch-condition`? @JacquesLucke, any specific reason why `-modernize-loop-convert` and `-modernize-pass-by-value` are only partial and a non-committed patch? Something you didn't have time yet, or something you are not sure it's a good change?

Jacques Lucke commented

2021-11-23 19:47:02 +01:00

Don't know about -modernize-loop-convert, but can check that.
We had inconclusive discussions about -modernize-pass-by-value in chat. I can update the patch to latest master and submit for review again.

Don't know about `-modernize-loop-convert`, but can check that. We had inconclusive discussions about `-modernize-pass-by-value` in chat. I can update the patch to latest master and submit for review again.

Philipp Oeser removed the

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Code quality day: Clang-Tidy #78535