Conditionally compile the parts of clang-tidy which depend on the static
analyzer.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Do builds even work properly if the CSA is not build right now?
clang-tidy/tool/CMakeLists.txt | ||
---|---|---|
32 ↗ | (On Diff #166392) | Is the MPI module remove from the list of available checks (clang-tidy -list-checks) with that? |
@JonasToth Sorry, I don't know what's unclear. I'm so surprised by your question that I think maybe I'm missing something. I thought the commit message and the patch itself are clear. Am I missing something?
Currently you can only build clang-tidy if you build the static analyzer.
This patch fixes that so that even if you do not build the static analyzer, you can build clang-tidy. That is the purpose of this patch, as in the title.
Am I missing something? Or did I misunderstand your comment?
clang-tidy/tool/CMakeLists.txt | ||
---|---|---|
32 ↗ | (On Diff #166392) | Yes. |
#ifdef hell is usually messy and is a source of problems.
May i ask what is the motivation for this change?
Sorry for formulating it unclear. I was curious if you could currently break the clang-tidy builds if you deactivate building CSA and that it might result in some dangling stuff or build errors.
But that clang-tidy is deactivated totally makes that impossible :)
I agree with @lebedev.ri that #ifdef in code might be a problem because its not constantly checked if the build still works if the analyzer is turned off and you might get bitrot.
Some of the clang-tidy stuff relies on Analysis/* from clang as well, e.g. the CFG class. Is this still included in builds with CSA off?
But that clang-tidy is deactivated totally makes that impossible :)
Yes. That should be clear by reading the patch.
Some of the clang-tidy stuff relies on Analysis/* from clang as well, e.g. the CFG class. Is this still included in builds with CSA off?
The Analysis includes are ifdef'd out in the patch. Have you read the patch?
Some of the clang-tidy stuff relies on Analysis/* from clang as well, e.g. the CFG class. Is this still included in builds with CSA off?
The Analysis includes are ifdef'd out in the patch. Have you read the patch?
Yes I did read the patch. bugprone-use-after-move utilizes CFG, there is no ifdefing in bugprone module. Thats why I am curious if the CFG is still part of a clang-build if the CSA is deactivated.
EDIT: let me clarify a little, its part of the build, but are there potential consequences, like something is missing ..., in the Analysis directory (because they ifdef something out as well).
Thanks, that at least makes it more obvious where you are getting confused.
See tools/clang/lib/CMakeLists.txt. It contains:
add_subdirectory(Analysis) # ... if(CLANG_ENABLE_STATIC_ANALYZER) add_subdirectory(StaticAnalyzer) endif()
- That is: Analysis and StaticAnalyzer are different.
CLANG_ENABLE_STATIC_ANALYZER refers to the latter.
- Also, something that may not have occurred to you: This patch does build. Obviously, if there were a problem with CFG after this patch, then it would not build.
Please keep both of those points in mind as you read the patch.
I expected, that you tested a build and running the unit tests as well ;)
What i suspected, that there are other parts directly in C++ source code that do an if CLANG_ENABLE_STATIC_ANALYZER to switch something on/off. I checked that now and it does not seem to be the case.
@alexfh should make the decision as the code-owner, but to me it makes sense to have clang-tidy without CSA.
Actually, I had not run the tests. Thanks for the reminder there. I extended the patch to enable the tests even if CSA is not available.
! In D52334#1242955, @JonasToth wrote:
... to me it makes sense to have clang-tidy without CSA.
Yep, it seems reasonable.
test/CMakeLists.txt | ||
---|---|---|
69 ↗ | (On Diff #166644) | There are some clang-tidy tests for the static analyzer integration (at least test/clang-tidy/static-analyzer.cpp and test/clang-tidy/static-analyzer-config.cpp). I would expect them to start failing when clang-tidy is built without static analyzer. Did you try running all the tests? |
test/CMakeLists.txt | ||
---|---|---|
69 ↗ | (On Diff #166644) | I realized that the tests were all excluded by the 'static-analyzer' switch too, so I've enabled them and disabled the ones which require the static-analyzer. |
Please wait for explicit approval from @alexfh
Am 26.09.2018 um 13:05 schrieb Stephen Kelly via Phabricator:
steveire added a comment.
Can I go ahead and merge this now?
Repository:
rCTE Clang Tools Extra
There has been a commit that seems related: https://reviews.llvm.org/rL343168
@steveire could you please take a look at how it affects this patch?
@JonasToth Once again - clangStaticAnalyzerCheckers is not clangAnalysis. Also, that commit changes the mpi plugin, which is excluded by this patch.
I think that we need some sort of developer documentation change that explains 1) that this option exists when configuring clang-tidy, and 2) that this impacts the MPI module as well as the static analyzer.
clang-tidy/CMakeLists.txt | ||
---|---|---|
29 ↗ | (On Diff #166825) | Indentation looks off here, same below. |
clang-tidy/ClangTidy.cpp | ||
93 ↗ | (On Diff #166825) | Please add comments to the end of the endif so that it's easier to know what the guarding condition is. Same elsewhere. |
@aaron.ballman Happy to add a note to the docs, but your request leaves me wondering where?
AFAIK, the fact that clang-tidy wasn't built at all when using CLANG_ENABLE_STATIC_ANALYZER is not documented, so I can't just update that.
Also AFAIK, other build switches don't have such exhaustive documentation that they mention things like this. Is this so important to document that it needs to be done here?
I don't think it is that important. I think it's just a bug that clang-tidy is entirely excluded from the build when not using CLANG_ENABLE_STATIC_ANALYZER. This commit fixes that bug, so it would be great to get a LGTM from someone/anyone so I can commit it.. This patch definitely does not need as much discussion as it is getting. It's just enabling the build of clang-tidy when not building the static analyzer.
clang-tidy/CMakeLists.txt | ||
---|---|---|
29 ↗ | (On Diff #166825) | Yes, I used the same indentation (none) as the previous code. I can indent this as you wish. |
I think a reasonable place would be docs/clang-tidy/index.rst in the "Getting Involved" area. We don't currently have any building instructions there, but this at least gets us started with something that may not be obvious to someone trying to get involved in the project.
Also AFAIK, other build switches don't have such exhaustive documentation that they mention things like this. Is this so important to document that it needs to be done here?
We do document these sort of switches in LLVM and Clang (e.g., https://llvm.org/docs/CMake.html). That we don't for extra tools is more a bug than a feature, IMO.
I don't think it is that important. I think it's just a bug that clang-tidy is entirely excluded from the build when not using CLANG_ENABLE_STATIC_ANALYZER. This commit fixes that bug, so it would be great to get a LGTM from someone/anyone so I can commit it.. This patch definitely does not need as much discussion as it is getting. It's just enabling the build of clang-tidy when not building the static analyzer.
It turns out that at least one of the people reviewing your patch disagrees. :-) It's hard to argue that this is fixing a bug when we never documented what you would get in the first place and the status quo is no static analyzer == no clang-tidy. I see this as implementing a new feature: allowing clang-tidy to be built without static analyzer support. Part of new features is documenting them.
I'm sorry that you find this process frustrating. I think the documentation is important here because we have package maintainers that need to know how to build things, and this mysterious undocumented flag changes the behavior of the resulting executable in pretty fundamental ways. It's unfortunate that we didn't document it when it was introduced, and now that we're touching it in interesting ways, we should fix that.
clang-tidy/CMakeLists.txt | ||
---|---|---|
29 ↗ | (On Diff #166825) | I think it's more clear to indent the body of the if statements, like you've switched to. Someday we should probably figure out cmake-format and try to make use of it to solve this kind of thing. |
clang-tidy/plugin/CMakeLists.txt | ||
32–33 ↗ | (On Diff #167584) | Indentation. |
I think a reasonable place would be docs/clang-tidy/index.rst in the "Getting Involved" area.
Thanks, that's at least actionable, but not very specific. I've added docs now. If they don't say what you want them to say, then please be specific about what you want them to say. There's no reason to have 5 more rounds of review here if being specific means only one more round is needed.
Are you going to 'accept' this patch eventually, or will I still be waiting for @alexfh once you're happy with it?
It's a reasonable, if terse, start to documenting how to build clang-tidy. Mostly some mechanical changes to format the new docs the same as the other docs in the file.
Are you going to 'accept' this patch eventually, or will I still be waiting for @alexfh once you're happy with it?
Once I'm happy, I will accept it. If @alexfh has further comments, then they can be addressed at that time (pre or post commit).
docs/clang-tidy/index.rst | ||
---|---|---|
340–341 ↗ | (On Diff #167615) | Double backticks around CLANG_ENABLE_STATIC_ANALYZER will not have -> will not be built with support for |
Once I'm happy, I will accept it. If @alexfh has further comments, then they can be addressed at that time (pre or post commit).
That's very reasonable, thanks.
LGTM! If you need this merged on your behalf, I can do so tomorrow or Monday, unless someone else gets to it first.
I've reverted in r343418 as the commit broke some bots.
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/37336
http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/24630
I also see that you have your own commit access, so I'll let you handle fixing up the failing tests and recommit. If there are not substantial changes required to fix the issue (e.g., it's just a matter of fixing up some tests), you're fine to fix them and commit without further review.