[clang] [clang] Only build static analyzer sources if requested (PR #71653)
https://github.com/tbaederr closed https://github.com/llvm/llvm-project/pull/71653 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Only build static analyzer sources if requested (PR #71653)
@@ -23,7 +23,9 @@ add_subdirectory(Tooling) add_subdirectory(DirectoryWatcher) add_subdirectory(Index) add_subdirectory(IndexSerialization) -add_subdirectory(StaticAnalyzer) +if(CLANG_ENABLE_STATIC_ANALYZER) petrhosek wrote: I agree with @llvm-beanz but I'd propose going even further and removing this option altogether. Rather than having separate `CLANG_ENABLE_` options for each tool which doesn't scale very well, we should encourage developers to use `LLVM_DISTRIBUTION_COMPONENTS` to control which tools to build. https://github.com/llvm/llvm-project/pull/71653 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Only build static analyzer sources if requested (PR #71653)
@@ -23,7 +23,9 @@ add_subdirectory(Tooling) add_subdirectory(DirectoryWatcher) add_subdirectory(Index) add_subdirectory(IndexSerialization) -add_subdirectory(StaticAnalyzer) +if(CLANG_ENABLE_STATIC_ANALYZER) llvm-beanz wrote: > Well yes, but I'm not introducing the option here, I'm just making it work. My argument here would be that if the option doesn't work, maybe we shouldn't fix it. In particular I think having the static analyzer be conditional in the testing tools makes for a more complicated testing matrix. If you disable the static analyzer in the build you also need to disable it in the testing tools and the test cases. This becomes a more complicated issue because changes in the AST library can impact correctness of the static analyzer, so should we really be supporting disabling the static analyzer from the testing? It seems to me that we should maybe support `CLANG_ENABLE_STATIC_ANALYZER` as a way to remove the static analyzer from the clang built product, but not from the build, testing tools, or test cases. https://github.com/llvm/llvm-project/pull/71653 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Only build static analyzer sources if requested (PR #71653)
@@ -23,7 +23,9 @@ add_subdirectory(Tooling) add_subdirectory(DirectoryWatcher) add_subdirectory(Index) add_subdirectory(IndexSerialization) -add_subdirectory(StaticAnalyzer) +if(CLANG_ENABLE_STATIC_ANALYZER) smeenai wrote: I agree with Chris in general. This case is a bit different because the static analyzer sources would be included in Clang otherwise instead of being a standalone target you could just not build (roughly analogous to `LLVM_TARGETS_TO_BUILD`), but what are the concrete savings (build time, binary size, etc.) from the option, to weigh against the added build complexity? I see some unguarded uses of the static analyzer libraries, e.g. `clang/tools/clang-check/CMakeLists.txt` unconditionally references `clangStaticAnalyzerFrontend`, so those would need to be adjusted as well if we went this route. It seems like that can get easily broken in the future as well though. https://github.com/llvm/llvm-project/pull/71653 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Only build static analyzer sources if requested (PR #71653)
@@ -23,7 +23,9 @@ add_subdirectory(Tooling) add_subdirectory(DirectoryWatcher) add_subdirectory(Index) add_subdirectory(IndexSerialization) -add_subdirectory(StaticAnalyzer) +if(CLANG_ENABLE_STATIC_ANALYZER) tbaederr wrote: Well yes, but I'm not introducing the option here, I'm just making it work. https://github.com/llvm/llvm-project/pull/71653 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Only build static analyzer sources if requested (PR #71653)
@@ -23,7 +23,9 @@ add_subdirectory(Tooling) add_subdirectory(DirectoryWatcher) add_subdirectory(Index) add_subdirectory(IndexSerialization) -add_subdirectory(StaticAnalyzer) +if(CLANG_ENABLE_STATIC_ANALYZER) llvm-beanz wrote: This approach slightly bothers me. One of the mistakes I think we (and by we I mean me) have made in the build system is allowing lots of things to be disabled from the configuration. In general I think it is desirable to build and test code always, but to support modularity in what gets built into the final products. I realize there is some contention between that approach having fast build and test cycles, but the tradeoff between fast and adequate coverage is not always an easy line to draw. (cc @petrhosek & @smeenai) https://github.com/llvm/llvm-project/pull/71653 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Only build static analyzer sources if requested (PR #71653)
llvmbot wrote: @llvm/pr-subscribers-clang Author: Timm Baeder (tbaederr) Changes There used to be a patch similar to this on Phabricator. I asked a long time ago if I can pick it up but the author told me they will work on it, which never happened. IIRC there also was a problem with this simple patch since some other component depends on the static analyzer being built. Let's see what CI says. --- Full diff: https://github.com/llvm/llvm-project/pull/71653.diff 1 Files Affected: - (modified) clang/lib/CMakeLists.txt (+3-1) ``diff diff --git a/clang/lib/CMakeLists.txt b/clang/lib/CMakeLists.txt index 1526d65795f8adf..e897fbb16c60d78 100644 --- a/clang/lib/CMakeLists.txt +++ b/clang/lib/CMakeLists.txt @@ -23,7 +23,9 @@ add_subdirectory(Tooling) add_subdirectory(DirectoryWatcher) add_subdirectory(Index) add_subdirectory(IndexSerialization) -add_subdirectory(StaticAnalyzer) +if(CLANG_ENABLE_STATIC_ANALYZER) + add_subdirectory(StaticAnalyzer) +endif() add_subdirectory(Format) if(CLANG_INCLUDE_TESTS) add_subdirectory(Testing) `` https://github.com/llvm/llvm-project/pull/71653 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [clang] Only build static analyzer sources if requested (PR #71653)
https://github.com/tbaederr created https://github.com/llvm/llvm-project/pull/71653 There used to be a patch similar to this on Phabricator. I asked a long time ago if I can pick it up but the author told me they will work on it, which never happened. IIRC there also was a problem with this simple patch since some other component depends on the static analyzer being built. Let's see what CI says. >From aec458976c007f2666c2a1939bf8e48b1840a3fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timm=20B=C3=A4der?= Date: Wed, 8 Nov 2023 11:43:22 +0100 Subject: [PATCH] [clang] Only build static analyzer sources if requested --- clang/lib/CMakeLists.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/clang/lib/CMakeLists.txt b/clang/lib/CMakeLists.txt index 1526d65795f8adf..e897fbb16c60d78 100644 --- a/clang/lib/CMakeLists.txt +++ b/clang/lib/CMakeLists.txt @@ -23,7 +23,9 @@ add_subdirectory(Tooling) add_subdirectory(DirectoryWatcher) add_subdirectory(Index) add_subdirectory(IndexSerialization) -add_subdirectory(StaticAnalyzer) +if(CLANG_ENABLE_STATIC_ANALYZER) + add_subdirectory(StaticAnalyzer) +endif() add_subdirectory(Format) if(CLANG_INCLUDE_TESTS) add_subdirectory(Testing) ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
