[PATCH] D129323: [llvm] add ztsd compression namespace

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: llvm/include/llvm/Support/Compression.h:48-51
+static constexpr int NoCompression = -5;
+static constexpr int BestSpeedCompression = 1;
+static constexpr int DefaultCompression = 5;
+static constexpr int BestSizeCompression = 12;
----------------
nit: perhaps this could be an enum with assigned values?


================
Comment at: llvm/lib/Support/Compression.cpp:141-149
+Error zstd::uncompress(StringRef InputBuffer,
+                       SmallVectorImpl<char> &UncompressedBuffer,
+                       size_t UncompressedSize) {
+  UncompressedBuffer.resize_for_overwrite(UncompressedSize);
+  Error E = zstd::uncompress(InputBuffer, UncompressedBuffer.data(),
+                             UncompressedSize);
+  UncompressedBuffer.truncate(UncompressedSize);
----------------
Hmm. I know there's going to be a lot more refactoring after this such that we pass around some abstract "compression" class that calls into abstract virtual methods rather than having various `llvm::zlib::uncompress`s everywhere, but it really bugs me how close to `zlib::uncompress` this looks and how much cleaner it would be if we could combine them (same with some of the other functions here). How difficult would it be to do that refactoring first before adding the zstd functions here? If it's too much work then I'm perfectly fine with LGTMing this one and saving that refactoring for later.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129323/new/

https://reviews.llvm.org/D129323