leonardchan added inline comments.
================
Comment at: clang-tools-extra/clangd/index/Serialization.cpp:193-194
}
- if (llvm::compression::zlib::isAvailable()) {
+ llvm::compression::CompressionAlgorithm *CompressionScheme =
+ new llvm::compression::ZlibCompressionAlgorithm();
+ CompressionScheme = CompressionScheme->whenSupported();
----------------
Will this leak?
================
Comment at: llvm/include/llvm/Support/Compression.h:56-58
+ virtual Error decompress(ArrayRef<uint8_t> Input,
+ SmallVectorImpl<uint8_t> &UncompressedBuffer,
+ size_t UncompressedSize) = 0;
----------------
Does the `uncompress` version of this just end up calling into the other
`uncompress` function? If so, we could probably just have one `decompress`
virtual method here and the one that accepts a `SmallVectorImpl` just calls
into the virtual `decompress` rather than have two virtual methods that would
do the same thing. It looks like you've done that in
`CompressionAlgorithmImpl`, but I think it could be moved here.
================
Comment at: llvm/include/llvm/Support/Compression.h:60-61
+
+ virtual CompressionAlgorithm *when(bool useCompression) = 0;
+ virtual CompressionAlgorithm *whenSupported() = 0;
+
----------------
Perhaps add some comments for these functions? At least for me, it's not
entirely clear what these are for.
================
Comment at: llvm/include/llvm/Support/Compression.h:66-67
+
+template <class CompressionAlgorithmType>
+class CompressionAlgorithmImpl : public CompressionAlgorithm {
+public:
----------------
Perhaps it would be simpler to just have the individual subclasses inherit from
`CompressionAlgorithm` rather than have them all go through
`CompressionAlgorithmImpl`? It looks like each child class with methods like
`getAlgorithmId` can just return the static values themselves rather than
passing them up to a parent to be returned. I think unless some static
polymorphism is needed here, CRTP might not be needed here.
================
Comment at: llvm/lib/Support/Compression.cpp:68
+
+void NoneCompressionAlgorithm::Compress(
+ ArrayRef<uint8_t> Input, SmallVectorImpl<uint8_t> &CompressedBuffer,
----------------
Does `NoneCompressionAlgorithm` imply there's no compression at all? If so, I
would think these methods should be empty.
================
Comment at: llvm/lib/Support/Compression.cpp:237-238
+llvm::compression::CompressionAlgorithmFromId(uint8_t CompressionSchemeId) {
+ llvm::compression::CompressionAlgorithm *CompressionScheme =
+ new llvm::compression::UnknownCompressionAlgorithm();
+ switch (CompressionSchemeId) {
----------------
Is the purpose of `UnknownCompressionAlgorithm` to be the default instance
here? If so, would it be better perhaps to just omit this and have an
`llvm_unreachable` in the `default` case below? I would assume users of this
function should just have the right compression scheme ID they need and any
error checking on if something is a valid ID would be done before calling this.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130516/new/
https://reviews.llvm.org/D130516
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
