Modernize clang-tidy setup #1951
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bbannier
force-pushed
the
topic/bbannier/cmake-clang-tidy
branch
4 times, most recently
from
acbe6a9 to
65fb497
Compare
rsmmr
previously approved these changes
Jan 7, 2025
This patch brings our clang-tidy setup up to date. Much of this was built with `./ci/run-ci` `in mind with extra suppressions hardcoded e.g., in `.clang-tidy.ignore`. With this patch we instead configure `.clang-tidy` files so they can suppress diagnostics from certain directories. We also set up 3rdparty code to work with CMake's `clang-tidy` integration[1]. We also globally suppress some diagnostics which we have no intention of addressing. [^1]: https://cmake.org/cmake/help/latest/prop_tgt/LANG_CLANG_TIDY.html#prop_tgt:%3CLANG%3E_CLANG_TIDY
This either switches to using `const char*` to signify guaranteed null termination, or locally suppress clang-tidy warnings about assuming null-terminated string_views.
This suppresses a `clang-tidy` lint suggesting to mark our default `main` function as not exported. This function is shipped as part of libhilti and intended to be available as a weak symbol.
In general `std::string_view` values do not need to be null-terminated, e.g., if they reference subranges in a bigger null-terminated string. We were previously passing `std::string_view` in a few places where we assumed them to be null-terminated, e.g., for passing them on to C APIs expecting null-terminated `const char*`. These worked well enough for us for now, but these APIs were generally unsafe. This patch switches to instead passing either `const char*` signalling proper, null-terminated strings, or by passing in a `const std::string&` where it should already be present in the caller.
`std::string_view` is a type which is designed to be (very) cheap to copy.
`clang-tidy` suggests adding extra parentheses when mixing e.g., addition and multiplication. Apply these suggestions (automatically).
It is not immediately clear to me when the previous implementation could move their values over. `clang-tidy` ended up flagging many of these as not moving and forcing copies. This patch rewrites the loops in these helpers to enforce iteration with move.
Many functions in the AST API were moving arguments only for them to ultimately be copied; `clang-tidy` diagnosed most of these. This patch cleans up the flagged useless passes by value and `std::move` without effect.
Instead of implementing this ourself instea use CMake with `CMAKE_CXX_CLANG_TIDY`/`CMAKE_C_CLANG_TIDY`.
This was already unused since we moved clang-format to a pre-commit hook.
bbannier
force-pushed
the
topic/bbannier/cmake-clang-tidy
branch
from
65fb497 to
616a88f
Compare
bbannier
commented
Jan 7, 2025
bbannier
force-pushed
the
topic/bbannier/cmake-clang-tidy
branch
from
356afc4 to
a050c87
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The idea of this PR is to bring us to date so that no
clang-tidydiagnostics are raised in my editor viaclangd, at least for my version of clang-tidy on macos-14.7.1. The way I went about this was to executeclang-tidyas part of the build, inbuild/Suppressions are now be consistently configured via
.clang-tidyconfigurations so they should work with the above setup. I fixed or suppressed some warnings I saw (often: only raised for newerclang-tidy).