Migrate to use clang-tidy v22#742
Conversation
* Add `--allow-no-checks` command-line argument for clang-tidy to turn off the error message "Error: no checks enabled." * Turns off overly aggressive checks in `.clang-tidy` configuration file. * Add inline `NOLINT` comments to turn off overly aggressive checks locally.
| sudo apt-get -qqy update | ||
| sudo apt-get -qy install clang-tidy-21 | ||
| sudo ln -fs "$(which clang-tidy-21)" "/usr/local/bin/clang-tidy" | ||
| sudo apt-get -qy install clang-tidy-22 |
There was a problem hiding this comment.
Upgrade to clang-tidy 22 for Linux.
| # qt6 versioning independently from the OS. | ||
| # brew install llvm@21 qt6 | ||
| brew install llvm@21 | ||
| brew install llvm@22 |
There was a problem hiding this comment.
Upgrade to clang-tidy 22 for macos.
| ${COMMON_COMPILER_OPTIONS} | ||
| -Wno-unused-value # for PYBIND11_EXPAND_SIDE_EFFECTS in pybind11.h | ||
| -Wno-noexcept-type # GCC | ||
| -Wno-elaborated-enum-base # for apple accelerate/veclib |
There was a problem hiding this comment.
This is to piggyback for another enhancement I am doing. -Welaborated-enum-base will be noisy.
| Checks: > | ||
| *,-clang-analyzer-alpha.*,-hicpp-*, | ||
| *, | ||
| -clang-analyzer-*, |
There was a problem hiding this comment.
Make it multiple lines and skip all of clang-analyzer-*.
| set(DO_CLANG_TIDY "${CLANG_TIDY_EXE}" | ||
| "-header-filter=/cpp/.\*" | ||
| "--extra-arg=-std=c++23" | ||
| # FIXME: Remove --allow-no-checks in the future after figuring out |
There was a problem hiding this comment.
It is strange to have the Error: no checks enabled. with the clang-tidy v22 upgrade. I have to add --allow-no-checks to have tidy run through. We should revisit this in the future.
There was a problem hiding this comment.
Since we are using --allow-no-checks, it might be useful to add a CI step to print the enabled checks for visibility
yungyuc
requested review from
ExplorerRay,
KHLee529,
ThreeMonth03,
chestercheng and
tigercosmos
KHLee529
left a comment
KHLee529
left a comment
There was a problem hiding this comment.
LGTM but I have some questions left in comments.
| const ssize_t m = static_cast<ssize_t>(l.shape(0)); | ||
| const ssize_t n = static_cast<ssize_t>(b.shape(1)); | ||
| small_vector<size_t> y_shape{static_cast<size_t>(m), static_cast<size_t>(n)}; | ||
| const auto m = static_cast<ssize_t>(l.shape(0)); |
There was a problem hiding this comment.
Is it considered better to use type inference instead of explicit type declaration ? What's the reason?
There was a problem hiding this comment.
Yes and no. It is suggested by modernize-use-auto. This line should use auto because it is casting.
Except the scenarios listed in modernize-use-auto and few others, we should spell out type names and avoid auto. Type names are good for readability.
| { | ||
|
|
||
| enum SimdFeature | ||
| enum SimdFeature : std::uint8_t |
There was a problem hiding this comment.
Don't this need explicitly declared as enum class?
There was a problem hiding this comment.
No it does not: https://en.cppreference.com/cpp/language/enum
ExplorerRay
left a comment
ExplorerRay
left a comment
There was a problem hiding this comment.
The change in CI part looks good to me.
chestercheng
left a comment
chestercheng
left a comment
There was a problem hiding this comment.
Overall LGTM.
I also noticed Node 20 deprecation warnings in CI, so I opened #743 to track it.
4 participants
Reconfigure GitHub Actions to use clang-tidy v22.
--allow-no-checkscommand-line argument for clang-tidy to turn off the error message "Error: no checks enabled.".clang-tidyconfiguration file.NOLINTcomments to turn off overly aggressive checks locally.In addition, use
-std=c++23instead ofc++2b.The migration work is tedious review and update. Not a lot to talk about.