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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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,
----------------
dblaikie wrote:
> 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)
I posted a small-write up here: 
https://reviews.llvm.org/D130516#3688123


================
Comment at: llvm/lib/Support/Compression.cpp:46
+  case DebugCompressionType::None:
+    assert(false && "expected a compression type");
+    break;
----------------
dblaikie wrote:
> llvm_unreachable should be preferred over assert false ( https://llvm.org/docs/CodingStandards.html#assert-liberally discusses this)
Thanks for mentioning the doc. For this place I hesitated while thinking about llvm_unreachable but picked assert because the user can call the function with any arguments and P.Format==DebugCompressionType::None reachable. The function is supposed to be unreachable from call sites, though...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130506