Skip to content

Clang tidy include cleaner #13280

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion .clang-tidy
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
# We use pointers to aggregates in a couple of places, intentionally.
# void * would look weird.
Checks: '-bugprone-sizeof-expression'
Checks: '
-bugprone-sizeof-expression
'
10 changes: 10 additions & 0 deletions doc/manual/source/development/building.md
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,16 @@ To refresh pre-commit hook's config file, do the following:
1. Exit the development shell and start it again by running `nix develop`.
2. If you also use the pre-commit hook, also run `pre-commit-hooks-install` again.

#### Hooks

##### clang-tidy

Portions of the codebase are checked with [clang-tidy](https://clang.llvm.org/extra/clang-tidy/).

> clang-tidy is a clang-based C++ “linter” tool.

The current build system, Meson, by default outputs a `compile_commands.json` file which is used by all Clang tooling (clang-tidy, clangd etc..).

### VSCode

Insert the following json into your `.vscode/settings.json` file to configure `nixfmt`.
Expand Down
14 changes: 14 additions & 0 deletions maintainers/flake-module.nix
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,20 @@
''^tests/functional/lang/eval-fail-set\.nix$''
];
};
clang-tidy = {
enable = true;
# TODO: this requires meson to have been configured
# we could optionally wrap this in a script that runs meson first
# but for now let us keep it simple
#
# clang-tidy doesn't work well running it on multiple files
# TODO: change this to use run-clang-tidy.py
entry = "${pkgs.writeShellScript "clang-tidy-wrapper" ''
for file in "$@"; do
"${pkgs.clang-tools}/bin/clang-tidy" --fix -p ./build "$file"
Copy link
Contributor

Choose a reason for hiding this comment

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

meson has clang-tidy and clang-tidy-fix targets. Can't we reuse that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea likely will do that but that runs on all targets.
I'm exploring how to add it as a pre-commit as well.
(We might abandon this change for now as we go about fixing warnings)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth noting that I did go over the codebase a bit with clang-tidy as a part of #11839. Thanks for picking up that activity!

Though we'd need to settle on a good clang-tidy config first. A lot of the checks are rather opinionated.

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 was trying to emit profile information to see which checks are really slow, so we can get something fast and iterate but I was hitting: llvm/llvm-project#141639

(Not sure if you faced this)

done
''}";
};
clang-format = {
enable = true;
# https://github.com/cachix/git-hooks.nix/pull/532
Expand Down
2 changes: 1 addition & 1 deletion src/libcmd/include/nix/cmd/repl.hh
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ struct AbstractNixRepl
Bindings * autoArgs;

AbstractNixRepl(ref<EvalState> state)
: state(state)
: state(state), autoArgs(nullptr)
{ }

virtual ~AbstractNixRepl()
Expand Down
Loading