Skip to content

add clang-tidy support #19643

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 30, 2019
Merged

add clang-tidy support #19643

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2cc43a0
add clang-tidy support
JohnCoconut Apr 27, 2019
2c2fbeb
move clang_tidy_test up in travis yaml file
JohnCoconut Apr 27, 2019
e253680
use dump-config to generate .clang-tidy file
JohnCoconut Apr 27, 2019
e118215
use flexible way to find run-clang-tidy.py
JohnCoconut Apr 28, 2019
b5dde36
place clang-tidy check options in separate lines
JohnCoconut Apr 28, 2019
0d996af
clang_tidy_test target clean up
JohnCoconut Apr 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions .clang-tidy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
---
Checks: 'clang-diagnostic-*,clang-analyzer-*,-*,performance-faster-string-find,performance-for-range-copy,'
Copy link
Contributor

@crazyhappygame crazyhappygame Apr 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change to yaml multplie line string https://yaml-multiline.info/. In that way we can easily track changes

Checks: >
    -*,
    clang-analyzer-*,
    clang-diagnostic-*,
    performance-faster-string-find,
    performance-for-range-copy,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

Checks:          'clang-diagnostic-*,clang-analyzer-*,-*,performance-faster-string-find,performance-for-range-copy,'

The configuration above is generated by clang-tidy -header-filter='/(?!external)/.*' -checks='-*,performance-faster-string-find,performance-for-range-copy,' -warnings-as-errors='*' -dump-config

Checks: >
    -*,
    clang-analyzer-*,
    clang-diagnostic-*,
    performance-faster-string-find,
    performance-for-range-copy,

I just found that the above two are not the same. The order matters. -* will disable options ahead of it.

  • In the first config, clang-diagnostic-*,clang-analyzer-* is disabled.

  • The second config, placing -* at the front, will not disable clang-diagnostic-*,clang-analyzer-*.

WarningsAsErrors: '*'
HeaderFilterRegex: '/(?!external)/.*'
AnalyzeTemporaryDtors: false
FormatStyle: none
CheckOptions:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I propose to remove "CheckOptions:".
Any way default values for "CheckOptions:" were used

- key: cert-dcl16-c.NewSuffixes
value: 'L;LL;LU;LLU'
- key: cppcoreguidelines-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic
value: '1'
- key: google-readability-braces-around-statements.ShortStatementLines
value: '1'
- key: google-readability-function-size.StatementThreshold
value: '800'
- key: google-readability-namespace-comments.ShortNamespaceLines
value: '10'
- key: google-readability-namespace-comments.SpacesBeforeComments
value: '2'
- key: modernize-loop-convert.MaxCopySize
value: '16'
- key: modernize-loop-convert.MinConfidence
value: reasonable
- key: modernize-loop-convert.NamingStyle
value: CamelCase
- key: modernize-pass-by-value.IncludeStyle
value: llvm
- key: modernize-replace-auto-ptr.IncludeStyle
value: llvm
- key: modernize-use-nullptr.NullMacros
value: 'NULL'
- key: performance-faster-string-find.StringLikeClasses
value: 'std::basic_string'
- key: performance-for-range-copy.AllowedTypes
value: ''
- key: performance-for-range-copy.WarnOnAllAutoCopies
value: '0'
...
6 changes: 6 additions & 0 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@ matrix:
- secure: M5lyDs0qai15mWHzJdkh0WPfVJJmVZu6SWtYULxatukGPXVwoQvmEtYAwAW+iz6aM+tXksQ/mk6nW5L8UFbHm+n6yrsa5bZU9sGXjilPE8p8bLFYDmIbPRazU+E6pBP3J2CDoAm0XnWkiYQ8feTxKTo6ysLnHAEjyaHTw0+Q1GM=
sudo: required
language: cpp
# clang-tidy
- os: linux
dist: xenial
env: BUILD_TARGET=linux_clang_tidy
language: cpp
sudo: required
# mac_cmake
- os: osx
env: BUILD_TARGET=mac_cmake
Expand Down
3 changes: 3 additions & 0 deletions tools/travis-scripts/before-install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ function install_environement_for_pull_request()
if [ "$BUILD_TARGET" == "linux" ]; then
install_linux_environment
fi
if [ "$BUILD_TARGET" == "linux_clang_tidy" ]; then
install_linux_environment
fi
fi

if [ "$TRAVIS_OS_NAME" == "osx" ]; then
Expand Down
16 changes: 16 additions & 0 deletions tools/travis-scripts/run-script.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@ function build_linux()
cmake --build .
}

function build_linux_clang_tidy()
{
echo "Building clang tidy test on Linux ..."
cd $COCOS2DX_ROOT/build
mkdir -p clang-tidy-build
cd clang-tidy-build
cmake ../.. -DCMAKE_EXPORT_COMPILE_COMMANDS=on
python /usr/local/clang-7.0.0/share/clang/run-clang-tidy.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

cmake ../.. -DCMAKE_EXPORT_COMPILE_COMMANDS=on
python /usr/local/clang-7.0.0/share/clang/run-clang-tidy.py

Don't quite understand these codes. And the python script path is hard coded, it may be a problem when CI changes its environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apt search clang-tidy in travis ci and found the most recent version is 6. But the clang-tidy being used is 7. My guess is that clang-tidy is not installed by package manager. I am not sure how to find run-clang-tidy.py.

Or we can just copy one and store it in the repo, if you agree.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minggo
cmake ../.. -DCMAKE_EXPORT_COMPILE_COMMANDS=on
CMAKE_EXPORT_COMPILE_COMMANDS is used to generate compilation database. This is the easiest way to use clang-tidy with CMake
https://clang.llvm.org/docs/JSONCompilationDatabase.html

About hard-coded path
python /usr/local/clang-7.0.0/share/clang/run-clang-tidy.py
It can be changed to e.g.:
"python $(dirname $(dirname $(readlink -f `which clang-tidy`)))/share/clang/run-clang-tidy.py"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crazyhappygame Great. It works on Fedora Linux too. I didn't know clang-tidy shares top-level directory with its python script.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@crazyhappygame thanks for the explanation.

}

function build_mac()
{
NUM_OF_CORES=`getconf _NPROCESSORS_ONLN`
Expand Down Expand Up @@ -291,6 +301,12 @@ function run_pull_request()
build_linux
fi

# linux_clang_tidy_test
if [ $BUILD_TARGET == 'linux_clang_tidy' ]; then
genernate_binding_codes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that genernate_binding_codes should be removed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it is not needed.

build_linux_clang_tidy
fi

# android
if [ $BUILD_TARGET == 'android_cpp_ndk-build' ]; then
build_android_cpp_ndk-build
Expand Down