Skip to content

[clang-tidy][NFC] Add mention of running 'clang-tidy' on changes in Contributing.rst #148547

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vbvictor
Copy link
Contributor

@vbvictor vbvictor commented Jul 13, 2025

Follow up to #147793.

Originally suggested by @carlosgalvezp in #147793 (comment)

@llvmbot
Copy link
Member

llvmbot commented Jul 13, 2025

@llvm/pr-subscribers-clang-tools-extra

Author: Baranov Victor (vbvictor)

Changes

Follow up to #147793.

Originally suggested in #147793 (comment)


Full diff: https://github.com/llvm/llvm-project/pull/148547.diff

1 Files Affected:

  • (modified) clang-tools-extra/docs/clang-tidy/Contributing.rst (+23)
diff --git a/clang-tools-extra/docs/clang-tidy/Contributing.rst b/clang-tools-extra/docs/clang-tidy/Contributing.rst
index 9611c655886f2..66c0abadc6a40 100644
--- a/clang-tools-extra/docs/clang-tidy/Contributing.rst
+++ b/clang-tools-extra/docs/clang-tidy/Contributing.rst
@@ -657,6 +657,29 @@ directory.  The path to this directory is available in a lit test with the varia
 .. _FileCheck: https://llvm.org/docs/CommandGuide/FileCheck.html
 .. _test/clang-tidy/checkers/google/readability-casting.cpp: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/google/readability-casting.cpp
 
+
+Submitting a Pull Request
+-------------------------
+
+Before submitting a pull request, contributors are encouraged to run
+:program:`clang-tidy` and :program:`clang-format` on their changes to ensure
+code quality and catch potential issues. While :program:`clang-tidy` is not
+currently enforced in CI, following this practice helps maintain code
+consistency and prevent common errors.
+
+Here's useful command to check your staged changes:
+
+.. code-block:: console
+
+  $ git diff --staged -U0 | ./clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py \
+      -j $(nproc) -path build/ -p1 -only-check-in-db
+  $ git clang-format
+
+Note that some warnings may be false positives or require careful consideration
+before fixing. Use your judgment and feel free to discuss in the pull request
+if you're unsure about a particular warning.
+
+
 Out-of-tree check plugins
 -------------------------
 

@llvmbot
Copy link
Member

llvmbot commented Jul 13, 2025

@llvm/pr-subscribers-clang-tidy

Author: Baranov Victor (vbvictor)

Changes

Follow up to #147793.

Originally suggested in #147793 (comment)


Full diff: https://github.com/llvm/llvm-project/pull/148547.diff

1 Files Affected:

  • (modified) clang-tools-extra/docs/clang-tidy/Contributing.rst (+23)
diff --git a/clang-tools-extra/docs/clang-tidy/Contributing.rst b/clang-tools-extra/docs/clang-tidy/Contributing.rst
index 9611c655886f2..66c0abadc6a40 100644
--- a/clang-tools-extra/docs/clang-tidy/Contributing.rst
+++ b/clang-tools-extra/docs/clang-tidy/Contributing.rst
@@ -657,6 +657,29 @@ directory.  The path to this directory is available in a lit test with the varia
 .. _FileCheck: https://llvm.org/docs/CommandGuide/FileCheck.html
 .. _test/clang-tidy/checkers/google/readability-casting.cpp: https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/test/clang-tidy/checkers/google/readability-casting.cpp
 
+
+Submitting a Pull Request
+-------------------------
+
+Before submitting a pull request, contributors are encouraged to run
+:program:`clang-tidy` and :program:`clang-format` on their changes to ensure
+code quality and catch potential issues. While :program:`clang-tidy` is not
+currently enforced in CI, following this practice helps maintain code
+consistency and prevent common errors.
+
+Here's useful command to check your staged changes:
+
+.. code-block:: console
+
+  $ git diff --staged -U0 | ./clang-tools-extra/clang-tidy/tool/clang-tidy-diff.py \
+      -j $(nproc) -path build/ -p1 -only-check-in-db
+  $ git clang-format
+
+Note that some warnings may be false positives or require careful consideration
+before fixing. Use your judgment and feel free to discuss in the pull request
+if you're unsure about a particular warning.
+
+
 Out-of-tree check plugins
 -------------------------
 

@EugeneZelenko
Copy link
Contributor

Clang-Tidy pull requests are usually small, so is -j really necessary?

Copy link
Contributor

@carlosgalvezp carlosgalvezp left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@vbvictor
Copy link
Contributor Author

vbvictor commented Jul 13, 2025

Clang-Tidy pull requests are usually small, so is -j really necessary?

I may say why not to use all cores when we can.
If PR only touches 1 file, clang-tidy-diff will not load other cores.
If PR touches multiple files, the user can have N times faster linting time, which is always great.

To put into perspective, average lint time of a single .cpp check-file is ~20-24 seconds on my machine, so if it is a refactor change in multiple checks at once -j can make a huge difference.

@vbvictor
Copy link
Contributor Author

vbvictor commented Jul 13, 2025

IMO, we should implicitly call multiprocessing.cpu_count() in run-clang-tidy and clang-tidy-diff and use that number if user didn't specify -j explicitly.

P.S. running lint over all clang-tidy is not so fast with single core now:)

$ time run-clang-tidy -p build/ clang-tools-extra/clang-tidy/
real    5m27.460s
user    163m43.110s
sys     2m5.180s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants