Code quality day: Clang-Tidy #78535
Open
opened 2020-07-02 15:38:35 +02:00 by Sergey Sharybin
·
16 comments
No Branch/Tag Specified
main
blender-v4.0-release
universal-scene-description
blender-v3.6-release
temp-sculpt-dyntopo
temp-sculpt-brush-channel
blender-v3.3-release
asset-browser-frontend-split
brush-assets-project
asset-shelf
anim/armature-drawing-refactor-3
temp-sculpt-dyntopo-hive-alloc
tmp-usd-python-mtl
tmp-usd-3.6
blender-v3.5-release
blender-projects-basics
blender-v2.93-release
temp-sculpt-attr-api
realtime-clock
sculpt-dev
gpencil-next
bevelv2
microfacet_hair
xr-dev
principled-v2
v3.6.4
v3.6.3
v3.3.11
v3.6.2
v3.3.10
v3.6.1
v3.3.9
v3.6.0
v3.3.8
v3.3.7
v2.93.18
v3.5.1
v3.3.6
v2.93.17
v3.5.0
v2.93.16
v3.3.5
v3.3.4
v2.93.15
v2.93.14
v3.3.3
v2.93.13
v2.93.12
v3.4.1
v3.3.2
v3.4.0
v3.3.1
v2.93.11
v3.3.0
v3.2.2
v2.93.10
v3.2.1
v3.2.0
v2.83.20
v2.93.9
v3.1.2
v3.1.1
v3.1.0
v2.83.19
v2.93.8
v3.0.1
v2.93.7
v3.0.0
v2.93.6
v2.93.5
v2.83.18
v2.93.4
v2.93.3
v2.83.17
v2.93.2
v2.93.1
v2.83.16
v2.93.0
v2.83.15
v2.83.14
v2.83.13
v2.92.0
v2.83.12
v2.91.2
v2.83.10
v2.91.0
v2.83.9
v2.83.8
v2.83.7
v2.90.1
v2.83.6.1
v2.83.6
v2.90.0
v2.83.5
v2.83.4
v2.83.3
v2.83.2
v2.83.1
v2.83
v2.82a
v2.82
v2.81a
v2.81
v2.80
v2.80-rc3
v2.80-rc2
v2.80-rc1
v2.79b
v2.79a
v2.79
v2.79-rc2
v2.79-rc1
v2.78c
v2.78b
v2.78a
v2.78
v2.78-rc2
v2.78-rc1
v2.77a
v2.77
v2.77-rc2
v2.77-rc1
v2.76b
v2.76a
v2.76
v2.76-rc3
v2.76-rc2
v2.76-rc1
v2.75a
v2.75
v2.75-rc2
v2.75-rc1
v2.74
v2.74-rc4
v2.74-rc3
v2.74-rc2
v2.74-rc1
v2.73a
v2.73
v2.73-rc1
v2.72b
2.72b
v2.72a
v2.72
v2.72-rc1
v2.71
v2.71-rc2
v2.71-rc1
v2.70a
v2.70
v2.70-rc2
v2.70-rc
v2.69
v2.68a
v2.68
v2.67b
v2.67a
v2.67
v2.66a
v2.66
v2.65a
v2.65
v2.64a
v2.64
v2.63a
v2.63
v2.61
v2.60a
v2.60
v2.59
v2.58a
v2.58
v2.57b
v2.57a
v2.57
v2.56a
v2.56
v2.55
v2.54
v2.53
v2.52
v2.51
v2.50
v2.49b
v2.49a
v2.49
v2.48a
v2.48
v2.47
v2.46
v2.45
v2.44
v2.43
v2.42a
v2.42
v2.41
v2.40
v2.37a
v2.37
v2.36
v2.35a
v2.35
v2.34
v2.33a
v2.33
v2.32
v2.31a
v2.31
v2.30
v2.28c
v2.28a
v2.28
v2.27
v2.26
v2.25
Labels
Clear labels
This issue affects/is about backward or forward compatibility
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports
Apply labels
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
This issue affects/is about backward or forward compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Issues relating to security: https://wiki.blender.org/wiki/Process/Vulnerability_Reports
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Set Project
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
8 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#78535
Reference in New Issue
There is no content yet.
Delete Branch "%!s(<nil>)"
Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?
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:
WITH_CLANG_TIDY=ON
(optionally specifyCLANG_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
.clang-tidy
file, make Blender compiled without this warning..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 in11c4066159
)-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
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.Changed status from 'Needs Triage' to: 'Confirmed'
Added subscriber: @Sergey
Added subscriber: @dr.sybren
Added subscriber: @JacquesLucke
Added subscriber: @ZedDB
Added subscriber: @HooglyBoogly
Added subscriber: @sebbas
Added subscriber: @ankitm
*Files in
makesrna
were left out of clang tidy by using it with compiler. Here's what works without actually compiling Blender: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.xzOthers generate very less errors:
clang_tidy_log.log
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
3b77c670f0
. Lets find out which of the points we want/plan to address!Those I find are nice candidates to address:
Something which seems interesting to investigate:
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.
Added subscriber: @jim-man
Clang-Tidy 12 have been released
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?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.