<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Apr 12, 2017 at 1:11 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><span class="gmail-"><div dir="ltr">On Wed, Apr 12, 2017 at 1:07 PM Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr" class="gmail-m_4683323386076598785gmail_msg">I still do not think that compressing debug sections using the streaming interface in the linker is a good idea. </div></blockquote></span><div><br>Fair, for sure and I /think/ George agrees - this review is totally pending until a discussion has demonstrated a need for this code.<br> </div><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr" class="gmail-m_4683323386076598785gmail_msg">Even if you eventually need it, you probably should start with a simpler one (which is `compress`), </div></blockquote><div><br></div></span><div>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'?</div><div><div class="gmail-h5"><div><br></div></div></div></div></div></blockquote><div><br></div><div>I meant the `compress` function that zlib provides (<a href="http://zlib.net/manual.html#Utility">http://zlib.net/manual.html#Utility</a>).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><div class="gmail-h5"><div></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr" class="gmail-m_4683323386076598785gmail_msg">as you don't know whether you'll need it or not.</div><div class="gmail_extra gmail-m_4683323386076598785gmail_msg"><br class="gmail-m_4683323386076598785gmail_msg"><div class="gmail_quote gmail-m_4683323386076598785gmail_msg">On Wed, Apr 12, 2017 at 9:03 AM, George Rimar via Phabricator <span dir="ltr" class="gmail-m_4683323386076598785gmail_msg"><<a href="mailto:reviews@reviews.llvm.org" class="gmail-m_4683323386076598785gmail_msg" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br class="gmail-m_4683323386076598785gmail_msg"><blockquote class="gmail_quote gmail-m_4683323386076598785gmail_msg" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">grimar added inline comments.<br class="gmail-m_4683323386076598785gmail_msg">
<span class="gmail-m_4683323386076598785gmail_msg"><br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
================<br class="gmail-m_4683323386076598785gmail_msg">
Comment at: include/llvm/Support/<wbr>Compression.h:60<br class="gmail-m_4683323386076598785gmail_msg">
+  /// OutBufSize - size of internal buffer for decompression.<br class="gmail-m_4683323386076598785gmail_msg">
+  static Expected<std::unique_ptr<<wbr>StreamCompression>> create(size_t OutBufSize);<br class="gmail-m_4683323386076598785gmail_msg">
+<br class="gmail-m_4683323386076598785gmail_msg">
----------------<br class="gmail-m_4683323386076598785gmail_msg">
</span><span class="gmail-m_4683323386076598785gmail_msg">dblaikie wrote:<br class="gmail-m_4683323386076598785gmail_msg">
> 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 class="gmail-m_4683323386076598785gmail_msg">
</span>Done.<br class="gmail-m_4683323386076598785gmail_msg">
<span class="gmail-m_4683323386076598785gmail_msg"><br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
================<br class="gmail-m_4683323386076598785gmail_msg">
Comment at: include/llvm/Support/<wbr>Compression.h:67<br class="gmail-m_4683323386076598785gmail_msg">
+  /// Out - callback that allows user code to receive compressed data.<br class="gmail-m_4683323386076598785gmail_msg">
+  Error compress(ArrayRef<uint8_t> InChunk, bool IsLastChunk, OutFn Out);<br class="gmail-m_4683323386076598785gmail_msg">
+<br class="gmail-m_4683323386076598785gmail_msg">
----------------<br class="gmail-m_4683323386076598785gmail_msg">
dblaikie wrote:<br class="gmail-m_4683323386076598785gmail_msg">
> dblaikie wrote:<br class="gmail-m_4683323386076598785gmail_msg">
> > Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?<br class="gmail-m_4683323386076598785gmail_msg">
> 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 class="gmail-m_4683323386076598785gmail_msg">
> Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?<br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
</span>Yep, that simpler. Changed.<br class="gmail-m_4683323386076598785gmail_msg">
<span class="gmail-m_4683323386076598785gmail_msg"><br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
================<br class="gmail-m_4683323386076598785gmail_msg">
Comment at: include/llvm/Support/<wbr>Compression.h:67<br class="gmail-m_4683323386076598785gmail_msg">
+  /// Out - callback that allows user code to receive compressed data.<br class="gmail-m_4683323386076598785gmail_msg">
+  Error compress(ArrayRef<uint8_t> InChunk, bool IsLastChunk, OutFn Out);<br class="gmail-m_4683323386076598785gmail_msg">
+<br class="gmail-m_4683323386076598785gmail_msg">
----------------<br class="gmail-m_4683323386076598785gmail_msg">
</span>grimar wrote:<br class="gmail-m_4683323386076598785gmail_msg">
<span class="gmail-m_4683323386076598785gmail_msg">> dblaikie wrote:<br class="gmail-m_4683323386076598785gmail_msg">
> > dblaikie wrote:<br class="gmail-m_4683323386076598785gmail_msg">
> > > Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?<br class="gmail-m_4683323386076598785gmail_msg">
> > 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 class="gmail-m_4683323386076598785gmail_msg">
</span><span class="gmail-m_4683323386076598785gmail_msg">> > Should the OutFn be part of the StreamCompression object - rather than passed in for each chunk of compression?<br class="gmail-m_4683323386076598785gmail_msg">
><br class="gmail-m_4683323386076598785gmail_msg">
</span>> Yep, that simpler. Changed.<br class="gmail-m_4683323386076598785gmail_msg">
<span class="gmail-m_4683323386076598785gmail_msg">><br class="gmail-m_4683323386076598785gmail_msg">
><br class="gmail-m_4683323386076598785gmail_msg">
><br class="gmail-m_4683323386076598785gmail_msg">
> 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 class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
</span>Done.<br class="gmail-m_4683323386076598785gmail_msg">
<span class="gmail-m_4683323386076598785gmail_msg"><br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
================<br class="gmail-m_4683323386076598785gmail_msg">
Comment at: include/llvm/Support/<wbr>Compression.h:72<br class="gmail-m_4683323386076598785gmail_msg">
+<br class="gmail-m_4683323386076598785gmail_msg">
+  std::vector<uint8_t> OutBuff;<br class="gmail-m_4683323386076598785gmail_msg">
+<br class="gmail-m_4683323386076598785gmail_msg">
----------------<br class="gmail-m_4683323386076598785gmail_msg">
</span><span class="gmail-m_4683323386076598785gmail_msg">dblaikie wrote:<br class="gmail-m_4683323386076598785gmail_msg">
> Would SmallVector be useful here?<br class="gmail-m_4683323386076598785gmail_msg">
</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 class="gmail-m_4683323386076598785gmail_msg">
<span class="gmail-m_4683323386076598785gmail_msg"><br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
================<br class="gmail-m_4683323386076598785gmail_msg">
Comment at: unittests/Support/<wbr>CompressionTest.cpp:82<br class="gmail-m_4683323386076598785gmail_msg">
+  Â  In[I] = I & 255;<br class="gmail-m_4683323386076598785gmail_msg">
+  ArrayRef<uint8_t> InChunk(In);<br class="gmail-m_4683323386076598785gmail_msg">
+<br class="gmail-m_4683323386076598785gmail_msg">
----------------<br class="gmail-m_4683323386076598785gmail_msg">
</span><span class="gmail-m_4683323386076598785gmail_msg">dblaikie wrote:<br class="gmail-m_4683323386076598785gmail_msg">
> No need for this variable - you should be able to pass 'In' directly as the ArrayRef parameter and rely on implicit conversion.<br class="gmail-m_4683323386076598785gmail_msg">
</span>Done.<br class="gmail-m_4683323386076598785gmail_msg">
<span class="gmail-m_4683323386076598785gmail_msg"><br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
================<br class="gmail-m_4683323386076598785gmail_msg">
Comment at: unittests/Support/<wbr>CompressionTest.cpp:105-106<br class="gmail-m_4683323386076598785gmail_msg">
+  // 4. Check that decompressed data equals to compressed.<br class="gmail-m_4683323386076598785gmail_msg">
+  for (size_t I = 0; I < Size * 4; ++I)<br class="gmail-m_4683323386076598785gmail_msg">
+  Â  EXPECT_EQ((uint8_t)<wbr>Uncompressed[I], I & 255);<br class="gmail-m_4683323386076598785gmail_msg">
+}<br class="gmail-m_4683323386076598785gmail_msg">
----------------<br class="gmail-m_4683323386076598785gmail_msg">
</span><span class="gmail-m_4683323386076598785gmail_msg">dblaikie wrote:<br class="gmail-m_4683323386076598785gmail_msg">
> Rather than writing a for loop, can you test that the uncompressed data is equal to the original input data?<br class="gmail-m_4683323386076598785gmail_msg">
</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 class="gmail-m_4683323386076598785gmail_msg">
I changed to test against input chunk instead of calculating value again.<br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
<a href="https://reviews.llvm.org/D31943" rel="noreferrer" class="gmail-m_4683323386076598785gmail_msg" target="_blank">https://reviews.llvm.org/<wbr>D31943</a><br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
<br class="gmail-m_4683323386076598785gmail_msg">
</blockquote></div><br class="gmail-m_4683323386076598785gmail_msg"></div>
</blockquote></div></div></div></div>
</blockquote></div><br></div></div>