Page MenuHome

Code quality day: Clang-Tidy
Confirmed, NormalPublic

Description

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.

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 (requires Clang-Tidy 12, which hasn't been released yet)
  • -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 (requires Clang-Tidy 12, which hasn't been released yet)
  • -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 rB11c4066159e1)
  • -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

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.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Sybren A. Stüvel (sybren) updated the task description. (Show Details)
Sybren A. Stüvel (sybren) updated the task description. (Show Details)

*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:


Others generate very less errors:

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.

I've expanded the modernize category in rB3b77c670f01. 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

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.