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

[Cole Kissane via Phabricator via llvm-commits]

ckissane marked an inline comment as done.
ckissane added inline comments.


================
Comment at: llvm/include/llvm/Object/Decompressor.h:52-53
   uint64_t DecompressedSize;
+  compression::CompressionSpecRef CompressionScheme =
+      compression::CompressionSpecRefs::Zlib;
 };
----------------
dblaikie wrote:
> 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`.
note that `null if no decompression is requested/needed` falls under the pattern for a `CompressionSpec*` however as a decompressor *assumes* that it is not none, it could still make sense to make it a `CompressionImpl*` as you request, which is null if unsupported (and error) and non-null if it's provided by the consumeCompressedSectionHeader.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131992