[PATCH] D130506: [Support] Add llvm::compression::{isAvailable,compress,uncompress}

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/Support/Compression.h:40-47
+// Return true if LLVM was built with support (LLVM_ENABLE_ZLIB,
+// LLVM_ENABLE_ZSTD) for the specified compression format.
+bool isAvailable(DebugCompressionType Type);
+
+// Compress Input with the specified format P.Format. If Level is -1, use
+// *::DefaultCompression for the format.
+void compress(Params P, ArrayRef<uint8_t> Input,
----------------
It seems to me somewhat unfortunate that these will generally be called somewhat together (isAvailable then compress or isAvailable then decompress) but don't share state/end up switching over compression type, etc, twice.

I've mentioned on the other reviews (maybe we could/should have a discourse thread about this, rather than spreading conversation between alternatives, preliminary patches, etc) that I think something like:
```
CompressionAlgorithm *C = getCompressionAlgorithm(DebugCompressionType::Zlib);
if (C) {
  C->compress(level, input, output);
}
```
Would be pretty good to me - possible variant (mentioned on one of the other reviews) would be that this returns non-null even if the algorithm isn't available (in which case it could return by reference) so it can communicate other things (like the name of the build flag needed to enable that algorithm - so it can be written/mapped from the enum in one place, and used in various error messages for various tools, etc)


================
Comment at: llvm/lib/Support/Compression.cpp:46
+  case DebugCompressionType::None:
+    assert(false && "expected a compression type");
+    break;
----------------
llvm_unreachable should be preferred over assert false ( https://llvm.org/docs/CodingStandards.html#assert-liberally discusses this)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130506