phosek added a comment.

I think this patch should be broken into at least two:

1. Refactor `llvm/include/llvm/Support/Compression.h` and 
`llvm/lib/Support/Compression.cpp` to introduce a generic interface and use it 
throughout the codebase.
2. zstd support in `llvm/include/llvm/Support/Compression.h` including the 
CMake bits.

When uploading future changes, please also make sure to include full context.



================
Comment at: llvm/include/llvm/Support/Compression.h:67-87
+#if LLVM_ENABLE_ZSTD
+namespace internal = llvm::compression::zstd;
+#else
+namespace internal = llvm::compression::zlib;
+#endif
+
+namespace elf = llvm::compression::zlib;
----------------
I think think that this should be a runtime rather than compile time selection. 
Many (if not most) toolchain vendors would likely want to enable both zstd and 
zlib and choose the algorithm based an option or file content.

For example, in the case of profiles, we want to be able to read existing 
profiles that use zlib, but for newly created profiles we may want to use zstd 
if available.

To make it possible, I suspect we may want to wrap the existing functions into 
a class that implements a common interface. I expect we would also need a 
function to detect the compression type based on the buffer content.


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