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

[phosek]

LGTM

[dblaikie]

llvm/include/llvm/Support/Compression.h

25–31

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.

[ckissane]

@dblaikie Yes, the end goal is to make it configurable and eventually refactor to make a shared "compression class" interface where we have zlib and zstd implementations.
However, this patch/patch before it is only pointed at adding an implementation of code to call zstd in the first place. Also @MaskRay has an draft patch that demonstrates how this current setup could still be used in a runtime way.
Note to make that intention clear we have dropped the usage of namespace aliases at the request of @phosek and others.
Edit:
TLDR:
a refactor of compression to use a class interface system is out of scope IMO for this patch

[dblaikie]

In D129324#3658393, @ckissane wrote:

@dblaikie Yes, the end goal is to make it configurable and eventually refactor to make a shared "compression class" interface where we have zlib and zstd implementations.
However, this patch is only pointed at adding an implementation of code to call zstd in the first place. Also @MaskRay has an draft patch that demonstrates how this current setup could still be used in a runtime way.
Note to make that intention clear we have dropped the usage of namespace aliases at the request of @phosek and others.
Edit:
TLDR:
a refactor of compression to use a class interface system is out of scope IMO for this patch

Cool, thanks for the context!

llvm/include/llvm/Support/Compression.h

 
 namespace llvm {
 template <typename T> class SmallVectorImpl;
 class Error;
 class StringRef;
 
 namespace compression {
 
 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;
 
 bool isAvailable();
 
 void compress(StringRef InputBuffer, SmallVectorImpl<char> &CompressedBuffer,
               int Level = DefaultCompression);
 
 Error uncompress(StringRef InputBuffer, char *UncompressedBuffer,
                  size_t &UncompressedSize);
 
 Error uncompress(StringRef InputBuffer,
                  SmallVectorImpl<char> &UncompressedBuffer,
                  size_t UncompressedSize);
 
 } // End of namespace zlib
 
 namespace zstd {
 
+static constexpr StringRef AlgorithmName = "zstd";
 static constexpr int NoCompression = -5;
 static constexpr int BestSpeedCompression = 1;
 static constexpr int DefaultCompression = 5;
 static constexpr int BestSizeCompression = 12;
 
 bool isAvailable();
 
 void compress(StringRef InputBuffer, SmallVectorImpl<char> &CompressedBuffer,