Skip to content

[NFC] Fix clang-tidy issues#1378

Merged
Icohedron merged 11 commits into
llvm:mainfrom
Icohedron:clang-tidy-fix
Jul 19, 2026
Merged

[NFC] Fix clang-tidy issues#1378
Icohedron merged 11 commits into
llvm:mainfrom
Icohedron:clang-tidy-fix

Conversation

@Icohedron

@Icohedron Icohedron commented Jul 17, 2026

Copy link
Copy Markdown
Contributor
  • Modifies the .clang-tidy to separate the Checks into multiple lines for better readability, adds -llvm-header-guard to silence clang-tidy warnings/errors regarding OFFLOADTEST_ prefixes on header guards, and adds -misc-const-correctness to disable const correctness checks which have false-positives that differ across clang-tidy versions

  • The CMakeLists.txt has been modified to pass --header-filter=^${CMAKE_CURRENT_SOURCE_DIR}/.*) to clang-tidy so that it is restricted to scanning project source files. This prevents clang-tidy from scanning llvm-project headers.

  • All github workflows now specify -DOFFLOADTEST_USE_CLANG_TIDY=ON (except MacOS which does not have clang-tidy).

  • Code refactoring has been performed to eliminate all clang-tidy warnings/errors from the latest clang-tidy version of 22.1.8

Assisted by: Claude Opus 4.8

Icohedron and others added 4 commits July 17, 2026 13:09
Separate the Checks into multiple lines for better readability.
Add -llvm-header-guard to silence errors regarding OFFLOADTEST_ being prefixed on the header guards.
Use  --header-filter=^${CMAKE_CURRENT_SOURCE_DIR}/.*) to restrict clang-tidy to project source files only.

Co-authored-by: Justin Bogner <mail@justinbogner.com>
Comment thread include/Support/Pipeline.h

@bob80905 bob80905 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the PR builds, then I have no other concerns, LGTM

@bogner bogner left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This mostly looks fine. I don't particularly like the NOLINT suppression - maybe we should just disable tests that have false positives?

@Icohedron

Copy link
Copy Markdown
Contributor Author

This mostly looks fine. I don't particularly like the NOLINT suppression - maybe we should just disable tests that have false positives?

Can't do that because it's a build error and any machine can perform the build for any of the tests.
We either need to use NOLINT or disable clang-tidy altogether,

@Icohedron Icohedron changed the title [NFC] Fix clang-tidy issues [NFC] Fix clang-tidy issues, and disable clang-tidy on GitHub workflows Jul 17, 2026
@bogner

bogner commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

This mostly looks fine. I don't particularly like the NOLINT suppression - maybe we should just disable tests that have false positives?

Can't do that because it's a build error and any machine can perform the build for any of the tests. We either need to use NOLINT or disable clang-tidy altogether,

Couldn’t we put -misc-const-correctness in the config file so it just doesn’t run that check?

@Icohedron

Copy link
Copy Markdown
Contributor Author

More clang-tidy errors on the builders... I think clang-tidy should just be disabled on the runners.

This mostly looks fine. I don't particularly like the NOLINT suppression - maybe we should just disable tests that have false positives?

Can't do that because it's a build error and any machine can perform the build for any of the tests. We either need to use NOLINT or disable clang-tidy altogether,

Couldn’t we put -misc-const-correctness in the config file so it just doesn’t run that check?

Can try that. If there are more clang-tidy errors though, I think it'll be better to have clang-tidy off by default.

@Icohedron Icohedron changed the title [NFC] Fix clang-tidy issues, and disable clang-tidy on GitHub workflows [NFC] Fix clang-tidy issues Jul 17, 2026
@Icohedron

Copy link
Copy Markdown
Contributor Author

Merging now since the PR checks have finished and I don't see any more build or clang-tidy issues.

@Icohedron
Icohedron merged commit a311cbc into llvm:main Jul 19, 2026
64 of 79 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants