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

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to