Bug 171632 – [Tools] Add support for clang-tidy

• Component: Tools / Tests
• Status: NEW

[Don Olmstead]

Investigate the use of clang-tidy for a linter within WebKit.

[Don Olmstead]

To install clang-tidy you can use one of the releases, http://releases.llvm.org/download.html, or on Linux use one of the nightly packages through, http://apt.llvm.org/.

To run clang-tidy you need to be doing a CMake build using a Ninja generator and of course be using clang.

CMake can support clang-tidy through a CMake define, -DCMAKE_EXPORT_COMPILE_COMMANDS=ON. This will generate a compile_commands.json which is used by clang-tidy.

As an example the following command for JSCOnly on Linux will generate the compile_commands.json.

./Tools/Scripts/build-webkit --jsconly --cmakeargs="-DCMAKE_EXPORT_COMPILE_COMMANDS=ON"

After CMake is done you can just kill the command since the compile_commands.json will be generated by the time the build starts.

If there is a specific file you would like to analyze use the following command with clang-tidy 0.5 installed through a nightly package.

clang-tidy -checks='-*,modernize*' -header-filter='.*' -fix -format-style=webkit -p WebKitBuild/Release/ Source/WTF/wtf/CPUTime.cpp 

If you would like to analyze everything the following can be used. Since it runs on all files in the project you can output to a file. In this command headers are not scanned.

run-clang-tidy-5.0.py -p WebKitBuild/Release > clang-tidy-run-0.5.txt 2>&1

If you would like to apply modernize fixes you can run the following as an example.

clang-tidy -checks='-*,modernize*' -header-filter='.*' -fix -format-style=webkit -p WebKitBuild/Release/ Source/WTF/wtf/CPUTime.cpp

[Don Olmstead]

Created attachment 308997 [details]
Example modernize fixes

[Alexey Proskuryakov]

Last I checked, clang's idea of WebKit style was quite wrong. Has that changed?

[Jonathan Bedard]

(In reply to Alexey Proskuryakov from comment #3)
> Last I checked, clang's idea of WebKit style was quite wrong. Has that
> changed?

Not since I've last checked.  Although, I wouldn't call it 'quite wrong,' if memory serves, there were a few minor discrepancies with spacing between if statements, I believe.

But Looking through what Don is suggesting here, this isn't Clang's WebKit style.  In fact, looking at the changes made to the Optional class, it's looking to me like that file didn't adhere to our style guidelines in the first place.

Don, if you run the webkit style checker on these changes (./Tools/Scripts/check-webkit-style), what does it say?  I could see it complaining about some of the spacing changes in Source/WTF/wtf/Optional.h.  Remember that the WebKit style checker only checks code which has changed, so it's a pretty common occurrence for code in the tree to be in violation of the style guidelines.

[Don Olmstead]

Created attachment 309070 [details]
Clang format for WebKit style

Here is the formatting config for style=WebKit. It was obtained by running

clang-format -style=WebKit -dump-config > .clang-format

The different options can be seen at https://clang.llvm.org/docs/ClangFormatStyleOptions.html

[Jonathan Bedard]

I'm sorry, I misread your first command Don.  You are using Clang's WebKit style.

I looked into using clang's WebKit style a few months ago.  I'm not sure where clang is getting their specifications for WebKit style from, it was a surprise to our team that this was an option.  While clang's format options are close to what we use, they fall short with some spacing problems, which actually requires modification of clang's source to correction.  If I remember correctly, the patch you posted here actually has that issue:

constexpr constexpr_optional_base() __NOEXCEPT :, storage_(trivial_init){};

should be

constexpr constexpr_optional_base() __NOEXCEPT :, storage_(trivial_init) {};

The style checker in WebKit (./Tools/Scripts/check-webkit-style) probably flags this, no?

[Don Olmstead]

(In reply to Jonathan Bedard from comment #6)
> I'm sorry, I misread your first command Don.  You are using Clang's WebKit
> style.
> 
> I looked into using clang's WebKit style a few months ago.  I'm not sure
> where clang is getting their specifications for WebKit style from, it was a
> surprise to our team that this was an option.  While clang's format options
> are close to what we use, they fall short with some spacing problems, which
> actually requires modification of clang's source to correction.  If I
> remember correctly, the patch you posted here actually has that issue:
> 
> constexpr constexpr_optional_base() __NOEXCEPT :, storage_(trivial_init){};
> 
> should be
> 
> constexpr constexpr_optional_base() __NOEXCEPT :, storage_(trivial_init) {};
> 
> The style checker in WebKit (./Tools/Scripts/check-webkit-style) probably
> flags this, no?

Yep it does flag that particular instance. Adding the diff and style checker results momentarily.

[Don Olmstead]

Created attachment 309081 [details]
Optional.h run through clang-format

Using the dumped .clang-format and running the following command

clang-format -i Source/WTF/Optional.h

I ran it directly on the file as Optional.h has a number of style violations already.

[Don Olmstead]

Created attachment 309082 [details]
Style checker results

Here are the results of running it through the style checker.

[Ryosuke Niwa]

I'm not certain I like the new format better. In general, I don't like code churns like this unless they explicitly violate our code style guideline: https://webkit.org/code-style-guidelines/

[Don Olmstead]

(In reply to Ryosuke Niwa from comment #10)
> I'm not certain I like the new format better. In general, I don't like code
> churns like this unless they explicitly violate our code style guideline:
> https://webkit.org/code-style-guidelines/

Clang-tidy comes with a tool for running on a diff and not the whole file https://github.com/llvm-mirror/clang-tools-extra/blob/master/clang-tidy/tool/clang-tidy-diff.py

[Don Olmstead]

Created attachment 314089 [details]
Clang tidy headers + clang format

ran the following commands

python "C:\Program Files\LLVM\share\clang\run-clang-tidy.py" -checks="modernize-deprecated-headers" -header-filter=".*" -fix -j 8 -format -p .  /Source/WTF/wtf/*

git diff -U0 --no-color HEAD^ | python 'C:\Program Files\LLVM\share\clang\clang-format-diff.py' -i -p1 -style WebKit

Had to do some cleanup where config.h got moved below but lets see how things look.

[Build Bot]

Attachment 314089 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Assertions.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WTF/wtf/CheckedArithmetic.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 53 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

(In reply to Don Olmstead from Bug 192710 Comment #6)
> Do we have any ideas on what clang-tidy rules should be run? I'd like to run
> it more often over the code base to see what it finds.
> 
> Fujii is working on getting clang-cl working on Windows at which point we
> should be able to run it more.
> 
> https://bugs.webkit.org/show_bug.cgi?id=171632 is a tracking bug for more
> general support for it in WebKit.

For the recent clang-tidy bugs I've been filing and fixing, I've been focusing only on performance-* warnings (excluding performance-noexcept-* warnings since WebKit compiles with C++ exceptions disabled by default):

clang-tidy -header-filter=.* -checks='-*,performance-*,-performance-noexcept-*' ...

I haven't tried any other checks yet.

My original intent was to find unnecessary object copies, specifically in for loops, but there were lots of other easy fixes to prevent object copies as well.

I wrote a hacky Perl script that parses xcodebuild output and then runs clang-tidy on the result to find these issues (in addition to building my own local copy of llvm/clang since Xcode's toolchains don't come with clang-tidy).