[PATCH] D129324: [llvm] add compression AlgorithmName (s) to the zlib and zstd namespaces

Fri Jul 8 09:58:29 PDT 2022

dblaikie added inline comments.

================
Comment at: llvm/include/llvm/Support/Compression.h:25-31
 namespace zlib {

+static constexpr StringRef AlgorithmName = "zlib";
 static constexpr int NoCompression = 0;
 static constexpr int BestSpeedCompression = 1;
 static constexpr int DefaultCompression = 6;
 static constexpr int BestSizeCompression = 9;
----------------
I worry that this setup (a namespace that defines an interface) looks like it's setup for some kind of compile-time substitution (eg: `#define LLVM_COMPRESSION_TYPE zlib` then doing things equivalent to `compression::LLVM_COMPRESSION_TYPE::compress`) which I think was discussed earlier in the patch series and something I think I pushed back on. Compression default would likely be configurable, and which compression options are compiled in (if zlib is compiled in/available, if zstd is compiled in/available) would be configurable, but the choice would/should still be at runtime for ease of rollout, integration with different tools that might only support zlib, etc.

ie: if you want to abstract over compression algorithms this should be a runtime abstraction, not a build time one - and this code here looks a lot like it'd be built for build-time choice of compression algorithm, which I think is something we should not do.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129324/new/

https://reviews.llvm.org/D129324