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

[Rui Ueyama via llvm-commits]

On Wed, Apr 12, 2017 at 1:11 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> 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'?
>
>
I meant the `compress` function that zlib provides (
http://zlib.net/manual.html#Utility).


> 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/e60389dd/attachment-0001.html>