[PATCH] D31943: [Support] - Implemented zlib::StreamCompression class.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Thanks for working on this - it'll be great to have streaming compression APIs for LLVM (& especially in llvm-dwp - it could use streaming compression (& streaming decompression, but having one side's a great start/example!))



================
Comment at: include/llvm/Support/Compression.h:60
+  /// OutBufSize - size of internal buffer for decompression.
+  static Expected<std::unique_ptr<StreamCompression>> create(size_t OutBufSize);
+
----------------
It'd be tidier if this could be Expected<StreamCompression> instead (avoiding the unique_ptr indirection, etc) & helpful if the StreamCompression class was movable.


================
Comment at: include/llvm/Support/Compression.h:67
+  /// Out - callback that allows user code to receive compressed data.
+  Error compress(ArrayRef<uint8_t> InChunk, bool IsLastChunk, OutFn Out);
+
----------------
Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?


================
Comment at: include/llvm/Support/Compression.h:67
+  /// Out - callback that allows user code to receive compressed data.
+  Error compress(ArrayRef<uint8_t> InChunk, bool IsLastChunk, OutFn Out);
+
----------------
dblaikie wrote:
> Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?
Also "IsLastChunk" is probably a somewhat clunky API - might be nicer to have a separate "finalize" function for once all the chunks are done?


================
Comment at: include/llvm/Support/Compression.h:72
+
+  std::vector<uint8_t> OutBuff;
+
----------------
Would SmallVector be useful here?


================
Comment at: unittests/Support/CompressionTest.cpp:82
+    In[I] = I & 255;
+  ArrayRef<uint8_t> InChunk(In);
+
----------------
No need for this variable - you should be able to pass 'In' directly as the ArrayRef parameter and rely on implicit conversion.


================
Comment at: unittests/Support/CompressionTest.cpp:105-106
+  // 4. Check that decompressed data equals to compressed.
+  for (size_t I = 0; I < Size * 4; ++I)
+    EXPECT_EQ((uint8_t)Uncompressed[I], I & 255);
+}
----------------
Rather than writing a for loop, can you test that the uncompressed data is equal to the original input data?


https://reviews.llvm.org/D31943