ckissane added a comment. 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" 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
