[PATCH] D130516: compression classes

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Any chance this could be split up to be more directly comparable to https://reviews.llvm.org/D130458 ?



================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:32
+llvm::compression::CompressionAlgorithm *StringTableCompressionScheme =
+    new llvm::compression::ZlibCompressionAlgorithm();
+
----------------
We're generally trying to avoid global ctors in LLVM. So at most this should be a static local variable in a function that accesses the algorithm (though perhaps these "compression algorithm" classes shouldn't be accessible directly, and only through singleton accessors in the defined alongside - like there's no reason for LLVM to contain more than one instance of ZlibCompressionAlgorithm, I think?)


================
Comment at: llvm/include/llvm/Support/Compression.h:71
+
+  virtual bool supported() { return CompressionAlgorithmType::Supported(); }
+
----------------
Rather than `supported()` maybe the accessor functions could return nullptr when support isn't available?
```
if (CompressionAlgorithm *A = getZstdCompressionScheme())
```
etc.

Though I guess that doesn't allow for a default implementation - I guess an alternative function could be `CompressionAlgorithm& getCompressionSchemeOrNone(Zstd)` which always gives a valid `CompressionAlgorithm` by giving the do-nothing compression algorithm when the specified one is not available.

But I guess we don't generally want to silently fallback to null compression, because the streams we're producing always need to know if they have to emit headers, etc, or not? So maybe there's no need for a default?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130516