[Lldb-commits] [PATCH] D128465: Zstandard as a second compression method to LLVM

[Petr Hosek via Phabricator via lldb-commits]

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