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

[David Blaikie via llvm-commits]

On Wed, Apr 12, 2017 at 1:07 PM Rui Ueyama <ruiu at google.com> wrote:

> I still do not think that compressing debug sections using the streaming
> interface in the linker is a good idea.
>

Fair, for sure and I /think/ George agrees - this review is totally pending
until a discussion has demonstrated a need for this code.


> Even if you eventually need it, you probably should start with a simpler
> one (which is `compress`),
>

I'm not sure I follow what you mean by "simpler [streaming interface]
(which is 'compress')" - what does such an API look like/what are you
referring to by 'compress'?


> as you don't know whether you'll need it or not.
>
> On Wed, Apr 12, 2017 at 9:03 AM, George Rimar via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
> 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
>
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170412/9ed72541/attachment.html>