[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Probably easier to review if it's framed somewhat more like @MaskRay and my examples - backwards compatible, without fixing all the API users as we can discuss those separately.

Though it is useful to have one or two example uses - but having all the API uses being updated in one patch could lead us to discussing the same issues repeatedly if we try to review it all in one go.



================
Comment at: llvm/include/llvm/Object/Decompressor.h:52-53
   uint64_t DecompressedSize;
+  compression::CompressionSpecRef CompressionScheme =
+      compression::CompressionSpecRefs::Zlib;
 };
----------------
I think the member should be the CompressionImpl, rather than the specref - null if no decompression is requested/needed, and non-null if it's required/provided by the `consumeCompressedSectionHeader`.


================
Comment at: llvm/include/llvm/Support/Compression.h:35-36
+
+typedef CompressionSpec *CompressionSpecRef;
+typedef CompressionImpl *CompressionImplRef;
+
----------------
`Ref` when it's actually a pointer might be confusing - not sure if these add more value over using the raw pointers themselves?


================
Comment at: llvm/include/llvm/Support/Compression.h:38-39
+
+CompressionSpecRef getCompressionSpec(uint8_t Kind);
+CompressionSpecRef getCompressionSpec(CompressionKind Kind);
+CompressionSpecRef getSchemeDetails(CompressionImplRef Implementation);
----------------
Probably don't need both of these, just the one that takes the enum type?


================
Comment at: llvm/include/llvm/Support/Compression.h:40
+CompressionSpecRef getCompressionSpec(CompressionKind Kind);
+CompressionSpecRef getSchemeDetails(CompressionImplRef Implementation);
+
----------------
I don't think we need this function?


================
Comment at: llvm/include/llvm/Support/Compression.h:44
+  const CompressionKind Kind;
+  CompressionImpl *Implementation;
+  const StringRef Name;
----------------
This should probably be a const member & the CompressionImpl's probably immutable, but could be `const` too, just to be explicit


================
Comment at: llvm/include/llvm/Support/Compression.h:46
+  const StringRef Name;
+  const bool Supported;
+  const StringRef Status; // either "supported", or "unsupported: REASON"
----------------
Probably don't need this member - it should be communicated by the non-null-ness of `Implementation`?


================
Comment at: llvm/include/llvm/Support/Compression.h:48-50
+  const int BestSpeedLevel;
+  const int DefaultLevel;
+  const int BestSizeLevel;
----------------
These could/should probably go in the `Implementation` since they're not useful when the algorithm isn't available anyway?


================
Comment at: llvm/include/llvm/Support/Compression.h:87
+
+  CompressionSpecRef spec() { return getCompressionSpec(Kind); }
+
----------------
the imple could have a pointer back to the spec, rather than having to do another lookup/need another function? (though, if the levels are moved to the impl, maybe this API is not needed?)


================
Comment at: llvm/include/llvm/Support/Compression.h:95-98
+  static CompressionSpecRef Unknown;
+  static CompressionSpecRef None;
+  static CompressionSpecRef Zlib;
+  static CompressionSpecRef ZStd;
----------------
Generally we don't want more variables that need global constructors in LLVM - so these should probably be function-local statics in functions instead.
(I don't think we need a CompressionSpecRef for `Unknown` or `None`, though)


================
Comment at: llvm/lib/Object/Decompressor.cpp:50
+    return createError(
+        "Decompressor provided nullptr (None) CompressionScheme*");
+  if (!CompressionScheme->Implementation)
----------------
This probably isn't a useful error message for a user. And this code is unreachable/untestable, right? The above code would've already errored out on "unsupported compression type"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131992