ckissane added a comment. In D128465#3646601 <https://reviews.llvm.org/D128465#3646601>, @MaskRay wrote:
> In D128465#3646561 <https://reviews.llvm.org/D128465#3646561>, @ckissane > wrote: > >> In D128465#3646258 <https://reviews.llvm.org/D128465#3646258>, @MaskRay >> wrote: >> >>> In D128465#3646203 <https://reviews.llvm.org/D128465#3646203>, @ckissane >>> wrote: >>> >>>> In D128465#3642997 <https://reviews.llvm.org/D128465#3642997>, @MaskRay >>>> wrote: >>>> >>>>> As I mentioned, the proper approach is to add zstd functionality along >>>>> with the CMake change, instead of adding CMake to all llvm-project >>>>> components without a way to test them. >>>> >>>> @MaskRay, I have now done this and ran the ldd tests as requested: >>>> >>>> With LLVM_ENABLE_ZSTD=ON >>>> $ ninja check-lld >>>> Testing Time: 8.98s >>>> Unsupported : 17 >>>> Passed : 2638 >>>> Expectedly Failed: 1 >>>> With LLVM_ENABLE_ZSTD=OFF >>>> $ ninja check-lld >>>> Testing Time: 8.95s >>>> Unsupported : 17 >>>> Passed : 2638 >>>> Expectedly Failed: 1 >>> >>> I request that you abandon this patch and incorporate the llvm cmake part >>> into the llvm patch which you actually use zstd. >>> It is not appropriate to add zstd cmake to llvm-project components which >>> don't use zstd. >> >> Let me see if I understand this correctly: >> Are you saying that I should abandon this patch, and create a new patch that: >> >> - adds findZSTD.cmake >> - adds zstd to compression namespace >> - adds tests to CompressionTest.cpp >> - and does the **minimal** amount of cmake work to have the flags and test >> work >> - thus differing per component cmake config to patches like how you are >> doing in https://reviews.llvm.org/D129406 >> >> Is this correct? >> >> I realize though that then https://reviews.llvm.org/D129406 or similar would >> be "the [first] llvm patch which actually use[s] zstd" > > You can find a component in llvm which uses zstd (e.g. lib/Support). Add the > functionality with tests (e.g. a CompressionTest.cpp unittest) along with the > CMake/(optional Bazel, gn) changes. > Keep clang, clang-tools-extra, lld, etc unchanged. These components don't yet > use zstd and the CMake changes in this patch will be untestable. Got it!, this patch already has such tests (CompressionTest.cpp unittest) in lib/Support, so I should just remove the untestable cmake config. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
