dblaikie added a comment.
Probably easier to review if it's framed somewhat more like @MaskRay and my
examples - backwards compatible, without fixing all the API users as we can
discuss those separately.
Though it is useful to have one or two example uses - but having all the API
uses being updated in one patch could lead us to discussing the same issues
repeatedly if we try to review it all in one go.
================
Comment at: llvm/include/llvm/Object/Decompressor.h:52-53
uint64_t DecompressedSize;
+ compression::CompressionSpecRef CompressionScheme =
+ compression::CompressionSpecRefs::Zlib;
};
----------------
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`.
================
Comment at: llvm/include/llvm/Support/Compression.h:35-36
+
+typedef CompressionSpec *CompressionSpecRef;
+typedef CompressionImpl *CompressionImplRef;
+
----------------
`Ref` when it's actually a pointer might be confusing - not sure if these add
more value over using the raw pointers themselves?
================
Comment at: llvm/include/llvm/Support/Compression.h:38-39
+
+CompressionSpecRef getCompressionSpec(uint8_t Kind);
+CompressionSpecRef getCompressionSpec(CompressionKind Kind);
+CompressionSpecRef getSchemeDetails(CompressionImplRef Implementation);
----------------
Probably don't need both of these, just the one that takes the enum type?
================
Comment at: llvm/include/llvm/Support/Compression.h:40
+CompressionSpecRef getCompressionSpec(CompressionKind Kind);
+CompressionSpecRef getSchemeDetails(CompressionImplRef Implementation);
+
----------------
I don't think we need this function?
================
Comment at: llvm/include/llvm/Support/Compression.h:44
+ const CompressionKind Kind;
+ CompressionImpl *Implementation;
+ const StringRef Name;
----------------
This should probably be a const member & the CompressionImpl's probably
immutable, but could be `const` too, just to be explicit
================
Comment at: llvm/include/llvm/Support/Compression.h:46
+ const StringRef Name;
+ const bool Supported;
+ const StringRef Status; // either "supported", or "unsupported: REASON"
----------------
Probably don't need this member - it should be communicated by the
non-null-ness of `Implementation`?
================
Comment at: llvm/include/llvm/Support/Compression.h:48-50
+ const int BestSpeedLevel;
+ const int DefaultLevel;
+ const int BestSizeLevel;
----------------
These could/should probably go in the `Implementation` since they're not useful
when the algorithm isn't available anyway?
================
Comment at: llvm/include/llvm/Support/Compression.h:87
+
+ CompressionSpecRef spec() { return getCompressionSpec(Kind); }
+
----------------
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?)
================
Comment at: llvm/include/llvm/Support/Compression.h:95-98
+ static CompressionSpecRef Unknown;
+ static CompressionSpecRef None;
+ static CompressionSpecRef Zlib;
+ static CompressionSpecRef ZStd;
----------------
Generally we don't want more variables that need global constructors in LLVM -
so these should probably be function-local statics in functions instead.
(I don't think we need a CompressionSpecRef for `Unknown` or `None`, though)
================
Comment at: llvm/lib/Object/Decompressor.cpp:50
+ return createError(
+ "Decompressor provided nullptr (None) CompressionScheme*");
+ if (!CompressionScheme->Implementation)
----------------
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"?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D131992/new/
https://reviews.llvm.org/D131992
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
