[PATCH] D130516: [llvm] compression classes

[Leonard Chan via Phabricator via cfe-commits]

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