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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
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);
+
----------------
dblaikie wrote:
> It'd be tidier if this could be Expected<StreamCompression> instead (avoiding the unique_ptr indirection, etc) & helpful if the StreamCompression class was movable.
Done.


================
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:
> 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?
> Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?

Yep, that simpler. Changed.





================
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);
+
----------------
grimar wrote:
> dblaikie wrote:
> > 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?
> > Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?
> 
> Yep, that simpler. Changed.
> 
> 
> 
> Also "IsLastChunk" is probably a somewhat clunky API - might be nicer to have a separate "finalize" function for once all the chunks are done?

Done.


================
Comment at: include/llvm/Support/Compression.h:72
+
+  std::vector<uint8_t> OutBuff;
+
----------------
dblaikie wrote:
> Would SmallVector be useful here?
I am not sure honestly. I would expect a few kilobytes output buffer size is reasonable, may be even more (I think about a megabyte for LLD use case). May be it is ok to have little buffer for some cases. Done.


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


================
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);
+}
----------------
dblaikie wrote:
> Rather than writing a for loop, can you test that the uncompressed data is equal to the original input data?
Original input data is N input chunks in a row. So that anyways will be a loop here I think, I need to test N intervals.
I changed to test against input chunk instead of calculating value again.


https://reviews.llvm.org/D31943