[PATCH] D131992: [Support] compression proposal for a enum->spec->impl approach
Cole Kissane via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 17 11:44:48 PDT 2022
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
More information about the cfe-commits
mailing list