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