pre-commit: Add clang-tidy hook #2982

Holzhaus · 2020-08-01T10:55:50Z

Follow up to #2981 and replaces #2975. Unsure if we want this, but I think it makes sense.

This merges the clang-format and line-length hooks into a single one. Semantically, this makes way more sense. Also, this prevent multiple attempted commit that are rejected by pre-commit (pre-commit might skip the line-length hook if old clang-format hook already failed). Also, this removes any dependency on git-clang-format and implements the whole mechanism itself. It retrieves all added lines from the unified diff instead and is now capable of taking PRE_COMMIT_FROM_REF into account. By moving the git-related code into a separate githelper library, it's way easier to write additional python wrappers for new hooks, e.g. for clang-tidy.

xeruf · 2020-08-02T12:01:43Z

.clang-tidy

+---
+Checks:          '-*,modernize-use-nullptr,modernize-use-auto'
+WarningsAsErrors: ''
+HeaderFilterRegex: ''
+AnalyzeTemporaryDtors: false
+FormatStyle:     none
+...


Don't do this! This will disable all checks for a standard run of clang-tidy across the project. The scope of checks for the hook is intentionally very narrow to be uncontroversial, but this shouldn't be the default for a normal run.

You can still reenable the checks when running on the command line. We should strive to keep manual runs and the pre-commit invocations consistent. If we really add clang-tidy support, we should just add all desired checks to this config file IMHO.

No, please not. Then it starts warning about all sorts of things that it can't automatically fix in the hook. Just don't.

The main problem is that clang-tidy doesn't seem to completely restrict itself to the lines you limited it too, so it can quickly become very verbose.

we should just add all desired checks to this config file IMHO.

Then it starts warning about all sorts of things that it can't automatically fix in the hook. Just don't.

The word desired is key here 😄 Support for auto-fixing is not a prerequite for using it in pre-commit though. We already have other hooks that may fail without auto-fix (certain eslint rules, qsscheck, etc.). The point is that it prevents us from committing questionable code.

Also, if we add a clang-tidy config file to the repo it also enables IDE/editors to pick it up and already warn us while writing code with the configuration tailored to Mixxx.

Yes, currently my IDE is warning me about the clang tidy default checks, with this config this would all go away. If you really want a full-blown config then don't start with -*.
But maybe we can start with a minimal one, because I am pretty sure some checks will be controversial and thus unneccessarily delay the merge.

xeruf · 2020-08-03T22:12:38Z

.clang-tidy

+---
+Checks:          '-*,modernize-use-nullptr,modernize-use-auto'
+WarningsAsErrors: ''
+HeaderFilterRegex: ''
+AnalyzeTemporaryDtors: false
+FormatStyle:     none
+...


Yes, currently my IDE is warning me about the clang tidy default checks, with this config this would all go away. If you really want a full-blown config then don't start with -*.
But maybe we can start with a minimal one, because I am pretty sure some checks will be controversial and thus unneccessarily delay the merge.

xeruf · 2020-08-03T22:14:35Z

tools/clang_tidy.py

+    parser = argparse.ArgumentParser()
+    parser.add_argument("--from-ref", help="compare against git reference")
+    parser.add_argument("files", nargs="*", help="only check these files")
+    args = parser.parse_args(argv)
+
+    if not args.from_ref:
+        args.from_ref = os.getenv("PRE_COMMIT_FROM_REF") or os.getenv(
+            "PRE_COMMIT_SOURCE"
+        )


I see repetition ;P

daschuer

The symlink to the compile_commands.json reveals a conceptual issue.
The pre-commit hook checks the source code independent from a build. While the test targets are checking the single build configurations config.

I think we need a different approach here. clang-tidy should become a part of the build system and should be treated the same as other compiler warnings.
Clang-tidy is then a special "warning level"

This way we have a Clang-tidy check for all our build targets.

A CI check can than verify that a build does not produce new CI warnings. Jenkins can do this, so I guess Travis and Appveyor are able to do this as well.

Interesting:
https://github.com/zemasoft/clangformat-cmake

daschuer · 2020-08-09T06:45:19Z

CMakeLists.txt

@@ -319,6 +319,13 @@ if(CMAKE_VERSION VERSION_LESS "3.7.0")
  set(CMAKE_INCLUDE_CURRENT_DIR ON)
 endif()

+# Export compile-commands.json and symlink it into the source directory (needed
+# for clang-tidy pre-commit hook)
+set(CMAKE_EXPORT_COMPILE_COMMANDS ON)


This should be remain a user option.

What do you mean with a "user option"?

it only creates a json-file in the cmake build directory, I don't see a need to ever disable that.

daschuer · 2020-08-09T06:46:30Z

CMakeLists.txt

+set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
+file(CREATE_LINK
+  ${CMAKE_BINARY_DIR}/compile_commands.json
+  ${CMAKE_SOURCE_DIR}/compile_commands.json SYMBOLIC)


This clutters the working directory. I think we should look out for an alternative solution.

I struggled with this for hours, believe me. Supposedly adding the -p argument to clang-tidy allows you to specify a directory where the json resides, but it did not work for me at all. And even if it worked - how do you know the name of the cmake build directory? It can be different for everyone.
Furthermore, this also enables you to run clang-tidy manually without any setup.
And it's not like we have a super-tidy working directory now ^^

daschuer · 2020-08-09T06:48:18Z

tools/clang_format.py

@@ -0,0 +1,137 @@
+#!/usr/bin/env python3


Isn't this change obsolete now?

daschuer · 2020-08-09T06:57:16Z

tools/clang_tidy.py

@@ -0,0 +1,85 @@
+#!/usr/bin/env python3


This file need to integrate the changes from clang_format.py form master.

Holzhaus · 2020-08-15T20:03:13Z

I found out that this is uneeded with CMake. When running CMake you can easily do this:

$ cmake -DCMAKE_CXX_CLANG_TIDY=clang-tidy ..

Or even specify arguments:

$ cmake -DCMAKE_CXX_CLANG_TIDY='clang-tidy -checks=-*,readability-*'

Closing this.

daschuer · 2020-08-15T21:23:22Z

Can you describe this in the wiki?

xeruf · 2020-08-17T07:53:59Z

@Holzhaus but that way doesn't allow us to automatically update only changed lines?

xeruf · 2020-08-19T09:07:01Z

@Holzhaus please reopen. The whole point why I started this is to allow these things to be automatically fixed, which your solution doesn't do.

Btw, we definitely also want this check ;) http://clang.llvm.org/extra/clang-tidy/checks/readability-braces-around-statements.html

Holzhaus · 2020-08-19T09:40:54Z

The problem is that his doesn't work properly because it relies on the compile-commands.json. We have not way to check/ensure that this file is in sync with the source code. We'd also need to install all CMake dependencies into the pre-commit build vm on Travis.

I don't know how to solve this. I'd be in favor of adding a clang tidy config file to the repository and then relying on your editor to lint while typing, so that you can fix things manually. Using nullptr instead of NULL is not exactly rocket science, and since we don't want to do mass reformattings, replacing it automatically would be nice-to-have but is not imperative.

xeruf · 2020-08-19T10:01:12Z

We'd also need to install all CMake dependencies into the pre-commit build vm on Travis.

You can disable clang-tidy on CI - it was intended as a developer aid.

I'd be in favor of adding a clang tidy config file to the repository and then relying on your editor to lint while typing

Sure, I am already doing that. But as stated above, that file should be much more generous with its checks and not start with -*.

The problem is that his doesn't work properly because it relies on the compile-commands.json.

Maybe we can add this to pre-commit in a way that it is disabled by default and can be enabled with an environment variable?

Otherwise I have to keep this local. This was an option for me from the very beginning, though it is not convenient.

xeruf · 2020-08-19T10:03:24Z

Using nullptr instead of NULL is not exactly rocket science, and since we don't want to do mass reformattings, replacing it automatically would be nice-to-have but is not imperative.

Adding braces, replacing NULL with nullptr... all mundane things that we have tools for. I don't want to keep wasting time on such things if we can easily automate them.

Or as Uwe puts it: I just don't want to think about it!

Every time I go from review to IDE to fix such a thing is an unnecessary context switch.

xeruf · 2020-08-21T11:32:15Z

So, we can't create a pre-commit hook that is disabled by default, but we can implement the hook in such a way that it only runs when a specific environment variable is set. Thus we shift the responsibility of making the compilation database available to the user.

And if we get it working with clang-tidy, we could even use the environment variable not only as a flag, but it could contain the location of the compilation database, then we don't have to necessarily symlink it :)

Holzhaus added 2 commits July 31, 2020 18:52

pre-commit: Improve documentation for clang-format hook

5589ce8

Holzhaus added the code quality label Aug 1, 2020

Holzhaus marked this pull request as draft August 1, 2020 10:56

Holzhaus mentioned this pull request Aug 1, 2020

tools: Add clang tidy #2975

Closed

xeruf reviewed Aug 2, 2020

View reviewed changes

tools: Add various improvements for clang_format.py/githelper.py

1e7698d

xeruf suggested changes Aug 3, 2020

View reviewed changes

Holzhaus added 3 commits August 4, 2020 17:21

tools/clang-format.py: Use plain "w" mode for writing files

c91ed3f

tools/clang-format.py: Add support for --to-ref argument

3babcd7

pre-commit: Do not format JS files using clang-format

ff15b5b

Holzhaus force-pushed the pre-commit-clang-tidy branch from 74d0429 to 1cfd829 Compare August 4, 2020 16:24

Holzhaus added 3 commits August 5, 2020 10:52

tools/githelper: Add comment regarding unstaged changes

eae9aba

pre-commit: Add clang-tidy support

f916521

tools/clang_tidy.py: Add support for PRE_COMMIT_TO_REF

c58faef

Holzhaus force-pushed the pre-commit-clang-tidy branch from 1cfd829 to c58faef Compare August 5, 2020 08:56

daschuer requested changes Aug 9, 2020

View reviewed changes

Holzhaus closed this Aug 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pre-commit: Add clang-tidy hook #2982

pre-commit: Add clang-tidy hook #2982

Holzhaus commented Aug 1, 2020

xeruf Aug 2, 2020

Holzhaus Aug 2, 2020

xeruf Aug 3, 2020

xeruf Aug 3, 2020

Holzhaus Aug 3, 2020

xeruf Aug 3, 2020

xeruf Aug 3, 2020

xeruf Aug 3, 2020

daschuer left a comment

daschuer Aug 9, 2020

xeruf Aug 10, 2020

xeruf Aug 10, 2020

daschuer Aug 9, 2020

xeruf Aug 10, 2020

daschuer Aug 9, 2020

daschuer Aug 9, 2020

Holzhaus commented Aug 15, 2020

daschuer commented Aug 15, 2020

xeruf commented Aug 17, 2020

xeruf commented Aug 19, 2020

Holzhaus commented Aug 19, 2020

xeruf commented Aug 19, 2020

xeruf commented Aug 19, 2020

xeruf commented Aug 21, 2020 •

edited

Loading

pre-commit: Add clang-tidy hook #2982

pre-commit: Add clang-tidy hook #2982

Conversation

Holzhaus commented Aug 1, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus commented Aug 15, 2020

daschuer commented Aug 15, 2020

xeruf commented Aug 17, 2020

xeruf commented Aug 19, 2020

Holzhaus commented Aug 19, 2020

xeruf commented Aug 19, 2020

xeruf commented Aug 19, 2020

xeruf commented Aug 21, 2020 • edited Loading

xeruf commented Aug 21, 2020 •

edited

Loading