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

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/Object/Decompressor.h:52-53
   uint64_t DecompressedSize;
+  compression::CompressionSpecRef CompressionScheme =
+      compression::CompressionSpecRefs::Zlib;
 };
----------------
ckissane wrote:
> 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.
I don't follow, sorry. All the error handling is done in `consumeCompressedSectionHeader` - the only thing done after/outside that is one unconditional `CompressionScheme->Implementation->decompress` - so it doesn't need the `CompressionScheme` only the impl, which it's going to use unconditionally anyway. The Decompressor::decompress will never be called if the header failed to parse/if there isn't a decompressor ready to handle it.

At least so far as I can see from the code?


================
Comment at: llvm/lib/ProfileData/InstrProf.cpp:495
   return collectPGOFuncNameStrings(
-      NameStrs, compression::zlib::isAvailable() && doCompression, Result);
+      NameStrs, doCompression ? OptionalCompressionScheme : nullptr, Result);
 }
----------------
looks like this could be changed to pass the implementation, without the spec? (the caller doesn't need/use the spec)


================
Comment at: llvm/lib/Support/Compression.cpp:187
+
+CompressionSpec *getCompressionSpec(CompressionKind Kind) {
+  return getCompressionSpec(uint8_t(Kind));
----------------
Looks like this comment ( https://reviews.llvm.org/D131992#inline-1270577 ) got lost, as there's still two `getCompressionSpec` functions - please remove the one taking a raw uint8_t, leaving only the enum version. Callers should deal with validating that they're passing a valid enum to that function, and then `getCompressionSpec` can return a reference, instead of a pointer - since it'd only be passed a valid compression kind?

I think UnknownD (also, what's the `D` suffix meant to mean? I can't figure it out from context) can probably be removed, and callers can deal with that case themselves? 


================
Comment at: llvm/unittests/Support/CompressionTest.cpp:88-89
+TEST(CompressionTest, ZStd) {
+  CompressionSpec *CompressionScheme =
+      getCompressionSpec(CompressionKind::ZStd);
+  CompressionImpl *CompressionImplementation =
----------------
Probably don't need a named variable for this, since it's only used once on the next line? Roll the `getCompressionSpec` call into the line below?


================
Comment at: llvm/unittests/Support/CompressionTest.cpp:90
+      getCompressionSpec(CompressionKind::ZStd);
+  CompressionImpl *CompressionImplementation =
+      CompressionScheme->Implementation;
----------------
Across this whole patch I think long variable names are probably making things harder to read/glance at than helpful - especially for shorter lived variables (or ones where the scope cares mostly about the variable anyway, so it's not getting lost amongst other variables/intentions in the code) could you use shorter variable names? (even single letter names or acronyms would probably suffice here - but `Spec` and `Impl` might be good for the unit test?)

But also, like I said previously - it'd be good to pull out all the use case changes maybe into another patch - so they can be reviewed separately & this review can focus on the API design (though I realize they're connected - it's helpful to se the use case to understand the design impact, but maybe with one or two small examples, rather than having to change every usage in one go)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131992