Custom clang tidy plugin #30886
Merged
Custom clang tidy plugin #30886
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
jbytheway
force-pushed
the
custom_clang_tidy
branch
22 times, most recently
from
779e6db to
0ebc695
Compare
ZhilkinSerg
added
[C++]
jbytheway
force-pushed
the
custom_clang_tidy
branch
from
3b00cc0 to
98150c4
Compare
jbytheway
force-pushed
the
custom_clang_tidy
branch
from
98150c4 to
af585f4
Compare
This was previously working for my local LLVM build. Now I want it to work on Travis, so I have prepared a release of clang-tidy that's suitable to run there, and set it up to work with either version.
To find non-standard check_clang_tidy.py, was previously using an environment variable. Switch to a CMake option instead, so that it's not necessary to set the env var wherever you run the plugins tests.
Previously we would not flag uses of long if cv-qualified or a reference type. Now we will.
When variables had a type which was some template parameter which happened to be long, this caused an error to be reported. We don't actually care about such cases; instead we would prefer the error to appear at the point the template argument is specified.
Should not be using LONG_MIN et al if we're not using long variables.
The check was reporting errors for functions returning long when that was a template parameter. We shouldn't complain in this case.
We were reporting problems with deduced long return types (e.g. on lambdas) where we shouldn't.
Auto types which happen to be long shouldn't trigger a warning.
The analysis runs on compiler-generated member functions. We need to ignore variables in those.
When an instantiation of a function template returns long, we don't want to report that; if it was written as long it will have already been reported on the template.
size_t is fairly often what people really want when using unsigned int.
These had snuck in while I was getting all my other "removing long" PRs merged.
jbytheway
force-pushed
the
custom_clang_tidy
branch
from
66ad683 to
2d801ea
Compare
jbytheway
changed the title
|
OK, after being side-tracked for a while eliminating (Gorgon unhappy due to Makefile changes) |
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.
Summary
SUMMARY: Infrastructure "Use custom clang-tidy plugin for Cata-specific code checks"
Purpose of change
To permit us to enforce custom rules of style or convention via our own clang-tidy checks.
Describe the solution
I thought this would be easy, but it is rather convoluted.
clang-tidydoes not have native plugin support. The approach recommended by the clang devs is to build an entireclang-tidywith your custom checks embedded. This does not fit our use case well.Therefore, I forked clang-tidy and have published a version there which does have plugin support.
There are two ways this can be used:
clang-tidyand point Cataclysm at is. I have this working and have documented how to do it (see changes toDEVELOPER_TOOLING.mdin this PR).From Cata's perspective the way this works is
clang-tidy(this is an executable and some headers).LD_PRELOADto inject our plugin into theclang-tidyrun.cata-checks like any otherclang-tidychecks.So far I've just written one simple check that searches for variables declared
long, because we want to get rid of those (for the time being I haven't got rid of them, because I want to verify the check can find them).Still to do:
longcheck.long(done in Removing long (part 7): Various miscellaneous changes #31550, Removing long (part 6): Treat money / cash as an int #31531, Removing long (part 5): Clean up use of unsigned long in translations #31529, Removing long (part 4): Remove support for serializing longs #31483, Removing long (part 3): Use ints for invlets #31468, Removing long (part 2): Convert item charges to int #31453, Removing long (part 1): Treat chars, keys, and keycodes as int, not long #31432)Describe alternatives you've considered
My patched
clang-tidydoes actually support a-pluginsoption, so you don't have toLD_PRELOAD, but for people running with their ownclang-tidybuilds, it's easier to get theLD_PRELOADversion working, so that's what I've gone with. However, to run the tests of the plugin does require the-pluginsoption.Additional context
I might easily be taking this too far. This is adding a new dependency to Cata's Travis builds, which we might not want. On the other hand, nothing really terrible happens if it breaks; we just don't get our custom checks any more.
On the plus side, if we find this does work well, we can use it as evidence to help persuade the clang devs to include plugin support as standard. That would of course make it all much simpler.