[PATCH] D130506: [Support] Add llvm::compression::{getReasonIfUnsupported,compress,decompress}

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a subscriber: phosek.
MaskRay added inline comments.


================
Comment at: llvm/include/llvm/Support/Compression.h:78-81
+enum class Format {
+  Zlib, ///< zlib
+  Zstd, ///< Zstandard
+};
----------------
dblaikie wrote:
> I don't think it's worth adding this type - `DebugCompressionType` seems fine, and some functions that consume it (like compress/decompress) can have a precondition that it is not `None` for those functions.
A separate type was @ckissane  @leonardchan @phosek 's request.

(My original change was to reuse `DebugCompressionType` for all llvm-project compression. I just did not rename it.)

I think with the current code, this is a small-enough difference. Even if we decide to merge the types, the use sites will get minimum updates (just renaming).


================
Comment at: llvm/include/llvm/Support/Compression.h:94-106
+struct Params {
+  constexpr Params(Format F)
+      : Format(F), Level(F == Format::Zlib ? zlib::DefaultCompression
+                                           : zstd::DefaultCompression) {}
+  Params(DebugCompressionType Type) : Params(formatFor(Type)) {}
+
+  Format Format;
----------------
dblaikie wrote:
> Not sure if this structure is carrying its weight? The level would only be a parameter to `compress` and not `decompress`, right? (so `decompress` should only take `Format`, not `Params` (& maybe just take `DebugCompressionKind` if `Format` is dropped)) & then if the struct is only used to group two parameters (format and level) for one function call, maybe that's not worth it and we can just take two parameters in `compress`?
I agree `Level` is not useful for decompress. Let me use `Format` for `decompress`.

I will keep this structure because compress will share another parameter expressing the degree of concurrency, so its 3 parameters for `compress`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130506