Skip to content
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.

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

pre-commit: Add clang-tidy hook #2982

Closed
wants to merge 9 commits into from
7 changes: 7 additions & 0 deletions .clang-tidy
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
Checks: '-*,modernize-use-nullptr,modernize-use-auto'
WarningsAsErrors: ''
HeaderFilterRegex: ''
AnalyzeTemporaryDtors: false
FormatStyle: none
...
Comment on lines +1 to +7
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

30 changes: 17 additions & 13 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -74,17 +74,32 @@ repos:
stages:
- commit
- manual
- repo: local
hooks:
- id: clang-tidy
name: clang-tidy
entry: tools/clang_tidy.py
require_serial: true
verbose: true
stages:
- commit
- manual
language: system
files: \.(c|cc|cxx|cpp|frag|glsl|h|hpp|hxx|ih|ispc|ipp|java|js|m|mm|proto|vert)$
additional_dependencies:
- clang-tidy
- repo: local
hooks:
- id: clang-format
name: clang-format
entry: tools/clang_format_wrapper.py
description: "Run clang-format in two passes (reformat, then break long lines)"
entry: tools/clang_format.py
require_serial: true
stages:
- commit
- manual
language: python
files: \.(c|cc|cxx|cpp|frag|glsl|h|hpp|hxx|ih|ispc|ipp|java|js|m|mm|proto|vert)$
files: \.(c|cc|cxx|cpp|frag|glsl|h|hpp|hxx|ih|ispc|ipp|java|m|mm|proto|vert)$
additional_dependencies:
- clang-format
- repo: https://github.com/psf/black
Expand Down Expand Up @@ -114,14 +129,3 @@ repos:
stages:
- commit
- manual
- repo: local
hooks:
- id: line-length
name: line-length
description: Check for lines longer 100 and brakes them before 80.
entry: ./tools/line_length.py
stages:
- commit
- manual
language: python
files: \.(c|cpp|h)$
7 changes: 7 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

This should be remain a user option.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean with a "user option"?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

file(CREATE_LINK
${CMAKE_BINARY_DIR}/compile_commands.json
${CMAKE_SOURCE_DIR}/compile_commands.json SYMBOLIC)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 ^^


# Mixxx itself
add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
src/analyzer/analyzerbeats.cpp
Expand Down
137 changes: 137 additions & 0 deletions tools/clang_format.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
#!/usr/bin/env python3
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this change obsolete now?

# -*- coding: utf-8 -*-
import argparse
import logging
import os
import re
import subprocess
import sys
import tempfile
import typing

import githelper


# We recommend a maximum line length of 80, but do allow up to 100 characters
# if deemed necessary by the developer. Lines that exceed that limit will
# be wrapped after 80 characters automatically.
LINE_LENGTH_THRESHOLD = 100
BREAK_BEFORE = 80


def get_clang_format_config_with_columnlimit(rootdir, limit):
"""Create a temporary config with ColumnLimit set to 80."""
cpp_file = os.path.join(rootdir, "src/mixxx.cpp")
proc = subprocess.run(
["clang-format", "--dump-config", cpp_file],
capture_output=True,
text=True,
)
proc.check_returncode()
return re.sub(
r"(ColumnLimit:\s*)\d+", r"\g<1>{}".format(BREAK_BEFORE), proc.stdout
)


def run_clang_format_on_lines(rootdir, file_to_format, stylepath=None):
logger = logging.getLogger(__name__)

line_arguments = [
"--lines={}:{}".format(start, end)
for start, end in file_to_format.lines
]
assert line_arguments

logger.info("Reformatting %s...", file_to_format.filename)
filename = os.path.join(rootdir, file_to_format.filename)
cmd = [
"clang-format",
"--style=file",
# The --assume-filename argument sets the path for the .clang-format
# config file implcitly by assuming a different location of the file to
# format
"--assume-filename={}".format(
os.path.join(
stylepath if stylepath else rootdir, file_to_format.filename
)
),
*line_arguments,
]

with open(filename) as fp:
logger.debug("Executing: %r", cmd)
proc = subprocess.run(cmd, stdin=fp, capture_output=True, text=True)
try:
proc.check_returncode()
except subprocess.CalledProcessError:
logger.error(
"Error while executing command %s: %s", cmd, proc.stderr,
)
raise

if proc.stderr:
logger.error(proc.stderr)
with open(filename, mode="w") as fp:
fp.write(proc.stdout)


def main(argv: typing.Optional[typing.List[str]] = None) -> int:
logging.basicConfig(
format="[%(levelname)s] %(message)s", level=logging.INFO
)

logger = logging.getLogger(__name__)

parser = argparse.ArgumentParser()
parser.add_argument("--from-ref", help="use changes changes since commit")
parser.add_argument("--to-ref", help="use changes until commit")
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"
)

if not args.to_ref:
args.to_ref = os.getenv("PRE_COMMIT_TO_REF") or os.getenv(
"PRE_COMMIT_ORIGIN"
)

# Filter filenames
rootdir = githelper.get_toplevel_path()

# First pass: Format added/changed lines using clang-format
logger.info("First pass: Reformatting added/changed lines...")
files_with_added_lines = githelper.get_changed_lines_grouped(
from_ref=args.from_ref,
to_ref=args.to_ref,
filter_lines=lambda line: line.added,
include_files=args.files,
)
for changed_file in files_with_added_lines:
run_clang_format_on_lines(rootdir, changed_file)

# Second pass: Wrap long added/changed lines using clang-format
logger.info("Second pass: Breaking long added/changed lines...")
files_with_long_added_lines = githelper.get_changed_lines_grouped(
from_ref=args.from_ref,
to_ref=args.to_ref,
filter_lines=lambda line: line.added
and len(line.text) > LINE_LENGTH_THRESHOLD,
include_files=args.files,
)
config = get_clang_format_config_with_columnlimit(rootdir, BREAK_BEFORE)
with tempfile.TemporaryDirectory(prefix="clang-format") as tempdir:
# Create temporary config with ColumnLimit enabled
configfile = os.path.join(tempdir, ".clang-format")
with open(configfile, mode="w") as configfp:
configfp.write(config)

for changed_file in files_with_long_added_lines:
run_clang_format_on_lines(rootdir, changed_file, stylepath=tempdir)
return 0


if __name__ == "__main__":
sys.exit(main())
130 changes: 0 additions & 130 deletions tools/clang_format_wrapper.py

This file was deleted.

Loading