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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.

I still have reservations, but the discussion doesn't seem productive, so let's go with this. Some minor tweaks to make, if you like.



================
Comment at: llvm/include/llvm/Support/Compression.h:26-27
+// object file formats (e.g. ELF). This is a separate class as we may add new
+// compression::Format members for non-debugging purposes.
+enum class DebugCompressionType {
+  None, ///< No compression
----------------
MaskRay wrote:
> ckissane wrote:
> > ckissane wrote:
> > > I think a renaming would be good to keep this extra clear.
> > > Not a blocker though
> > > it could also be good to keep this enum closer to elf-related code.
> > Note that you state:
> > > Move llvm::DebugCompressionType from MC to Support to avoid Support->MC cyclic
> > > dependency. There are 40+ uses in llvm-project.
> > 
> > would moving both this enum and
> > ```
> > inline Format formatFor(DebugCompressionType Type) 
> > ```
> > into MC also resolve this?
> The `DebugCompressionType` rename change is what I want to avoid. The type may be reused by another object file format and there are ~40 uses in llvm-project which would need updating.
> 
> MC depends on Support, so Support cannot depend on MC (cyclic). The type is therefore moved to the lower level: Support.
I think it's probably reasonable to understand `DebugCompressionType` as an API level enum, not necessarily 1:1 with the actual value in the ELF format - it probably wouldn't be a bad idea to separate those at some point, but not the end of the world.


================
Comment at: llvm/include/llvm/Support/Compression.h:78-81
+enum class Format {
+  Zlib, ///< zlib
+  Zstd, ///< Zstandard
+};
----------------
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.


================
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;
----------------
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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130506