<div dir="ltr">I still do not think that compressing debug sections using the streaming interface in the linker is a good idea. Even if you eventually need it, you probably should start with a simpler one (which is `compress`), as you don't know whether you'll need it or not.</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 12, 2017 at 9:03 AM, George Rimar via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">grimar added inline comments.<br>
<span class=""><br>
<br>
================<br>
Comment at: include/llvm/Support/<wbr>Compression.h:60<br>
+  /// OutBufSize - size of internal buffer for decompression.<br>
+  static Expected<std::unique_ptr<<wbr>StreamCompression>> create(size_t OutBufSize);<br>
+<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> It'd be tidier if this could be Expected<StreamCompression> instead (avoiding the unique_ptr indirection, etc) & helpful if the StreamCompression class was movable.<br>
</span>Done.<br>
<span class=""><br>
<br>
================<br>
Comment at: include/llvm/Support/<wbr>Compression.h:67<br>
+  /// Out - callback that allows user code to receive compressed data.<br>
+  Error compress(ArrayRef<uint8_t> InChunk, bool IsLastChunk, OutFn Out);<br>
+<br>
----------------<br>
dblaikie wrote:<br>
> dblaikie wrote:<br>
> > Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?<br>
> Also "IsLastChunk" is probably a somewhat clunky API - might be nicer to have a separate "finalize" function for once all the chunks are done?<br>
> Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?<br>
<br>
</span>Yep, that simpler. Changed.<br>
<span class=""><br>
<br>
<br>
<br>
<br>
================<br>
Comment at: include/llvm/Support/<wbr>Compression.h:67<br>
+  /// Out - callback that allows user code to receive compressed data.<br>
+  Error compress(ArrayRef<uint8_t> InChunk, bool IsLastChunk, OutFn Out);<br>
+<br>
----------------<br>
</span>grimar wrote:<br>
<span class="">> dblaikie wrote:<br>
> > dblaikie wrote:<br>
> > > Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?<br>
> > Also "IsLastChunk" is probably a somewhat clunky API - might be nicer to have a separate "finalize" function for once all the chunks are done?<br>
</span><span class="">> > Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?<br>
><br>
</span>> Yep, that simpler. Changed.<br>
<span class="">><br>
><br>
><br>
> Also "IsLastChunk" is probably a somewhat clunky API - might be nicer to have a separate "finalize" function for once all the chunks are done?<br>
<br>
</span>Done.<br>
<span class=""><br>
<br>
================<br>
Comment at: include/llvm/Support/<wbr>Compression.h:72<br>
+<br>
+  std::vector<uint8_t> OutBuff;<br>
+<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Would SmallVector be useful here?<br>
</span>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.<br>
<span class=""><br>
<br>
================<br>
Comment at: unittests/Support/<wbr>CompressionTest.cpp:82<br>
+    In[I] = I & 255;<br>
+  ArrayRef<uint8_t> InChunk(In);<br>
+<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> No need for this variable - you should be able to pass 'In' directly as the ArrayRef parameter and rely on implicit conversion.<br>
</span>Done.<br>
<span class=""><br>
<br>
================<br>
Comment at: unittests/Support/<wbr>CompressionTest.cpp:105-106<br>
+  // 4. Check that decompressed data equals to compressed.<br>
+  for (size_t I = 0; I < Size * 4; ++I)<br>
+    EXPECT_EQ((uint8_t)<wbr>Uncompressed[I], I & 255);<br>
+}<br>
----------------<br>
</span><span class="">dblaikie wrote:<br>
> Rather than writing a for loop, can you test that the uncompressed data is equal to the original input data?<br>
</span>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.<br>
I changed to test against input chunk instead of calculating value again.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D31943" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D31943</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>