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

[Cole Kissane via Phabricator via llvm-commits]

ckissane added inline comments.


================
Comment at: llvm/include/llvm/Support/Compression.h:48
+
+static constexpr int NoCompression = -5;
+static constexpr int BestSpeedCompression = 1;
----------------
MaskRay wrote:
> leonardchan wrote:
> > nit: perhaps this could be an enum with assigned values?
> Just use `constexpr`. They are automatically internal linkage
I am simply matching the layout of the current zlib namespace here, what is your preference for handling this given that?


================
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;
----------------
ckissane wrote:
> MaskRay wrote:
> > leonardchan wrote:
> > > nit: perhaps this could be an enum with assigned values?
> > Just use `constexpr`. They are automatically internal linkage
> I am simply matching the layout of the current zlib namespace here, what is your preference for handling this given that?
I am simply matching the layout of the current zlib namespace here, what is your preference for handling this given that?


================
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);
----------------
leonardchan wrote:
> 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.
hmm, given the other nits about my simple mirroring of the layout of the zlib namespace perhaps we should shoot for a compressor class refactor of zlib prior...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129323