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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
