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

[Cole Kissane via Phabricator via cfe-commits]

ckissane marked 6 inline comments as done.
ckissane added inline comments.


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


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


================
Comment at: llvm/include/llvm/Support/Compression.h:87
+
+  CompressionSpecRef spec() { return getCompressionSpec(Kind); }
+
----------------
dblaikie wrote:
> 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?)
`levels are moved to the impl, [thus] this API is not needed`


================
Comment at: llvm/lib/Object/Decompressor.cpp:50
+    return createError(
+        "Decompressor provided nullptr (None) CompressionScheme*");
+  if (!CompressionScheme->Implementation)
----------------
ckissane wrote:
> dblaikie wrote:
> > 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"?
> true, it is essentially a repeat of the above
redundancy removed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131992