aaron.ballman added a comment. In https://reviews.llvm.org/D52334#1249875, @steveire wrote:
> @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. 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. ================ Comment at: clang-tidy/CMakeLists.txt:29 +if(CLANG_ENABLE_STATIC_ANALYZER) +target_link_libraries(clangTidy PRIVATE + clangStaticAnalyzerCore ---------------- steveire wrote: > aaron.ballman wrote: > > Indentation looks off here, same below. > Yes, I used the same indentation (none) as the previous code. I can indent > this as you wish. 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. ================ Comment at: clang-tidy/plugin/CMakeLists.txt:32-33 +if(CLANG_ENABLE_STATIC_ANALYZER) +target_link_libraries(clangTidyPlugin PRIVATE + clangTidyMPIModule +) ---------------- Indentation. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52334 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
