<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Aug 26, 2014 at 2:04 PM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>On 26 August 2014 14:30, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>


> This patch essentially supercedes one I sent out for review a few<br>
> weeks ago? (that had to play games with ownership/unique_ptrs around<br>
> these functions due to the split ownership, etc) and takes the stance<br>
> of "construct a new shallow MemoryBuffer rather than lying to 'share'<br>
> unique ownership"?<br>
<br>
</div>More or less. From my point of view the stance is that the interface<br>
should have the type that more directly corresponds to the interface<br>
semantics, not its implementation. For example<br>
<br>
* parseAssembly: without this patch it takes a MemoryBuffer to pass to<br>
a SourceMgr, but the fact that uses a SourceMgr should be an<br>
implementation detail and not change the interface.<br>
* parseBitcodeFile: It takes a MemoryBuffer because it is implemented<br>
on top of getLazyBitcodeModuleImpl, but that is also an implementation<br>
detail. We could have a separate non-lazy code path in the reader for<br>
example.<br></blockquote><div><br></div><div>Sure enough.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><br>
> Might be worth changing getLazyBitcodeModuleImpl to take a<br>
> unique_ptr<MemoryBuffer>&& before you commit this patch? (that way you<br>
> don't have to play games with get/release after the successful call)<br>
<br>
</div>You mean "unique_ptr<MemoryBuffer>&", right?</blockquote><div><br>Actually I did mean && - it's not uncommon for APIs to take by movement then fail to move-from in their failure path. Granted in the C++ standard library that failure is demonstrated by throwing an exception, whereas this has a return result.<br>
 </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> It actually looks easier<br>
to do that after my patch. I attached a followup patch.<br></blockquote><div><br>Sure - looks basically right, though the release in getLzyBitcodeModuleImpl is a bit scary. See my patch in the old review for another way to approach this - using an RAII cleanup and being able to take ownership of the buffer back from the BitcodeReader on the failure paths. I'm not sure it's the right answer, but might be worth tossing around some thoughts.<br>
<br>- David<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div></div>