<div dir="ltr"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">I realize your goals are somewhat more immediate...</blockquote><div><br></div><div>There's a lot of useful cleanup that can be done behind this interface so this isn't immediately blocking anything, but I want to start the discussion early so it doesn't become a blocker.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">The risk here is that it would be easy to miss the 'take()' call (& auto especially makes that risky...</blockquote><div><br></div><div>In transient_unique_ptr's defense that's really only going to shift your compile-time error a few lines: with no dereference operator and no implicit conversion you'll find out MAB isn't a unique_ptr really quickly. :)</div><div><br></div><div>I still like "fix the layering" the most, but don't have a good intuition for the impact of that yet. Failing that (2) seems fine to me.</div><div><br></div><div>-- Lang.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 11, 2017 at 11:29 AM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@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 dir="ltr"><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">(4) Move target registration out of Support so that it can include the<br>required definitions from MC.</blockquote><div><br></div></span><div>Yep -- left that one out. Chalk that up to the fever. This seems like an ideal solution if we can do it.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- Lang.</div></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 11, 2017 at 11:25 AM, Rafael Avila de Espindola <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 class="m_-4836685579913998378HOEnZb"><div class="m_-4836685579913998378h5">Lang Hames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> writes:<br>
<br>
> Hi All,<br>
><br>
> While trying to put together an MC-based assembler the other day, I again<br>
> encountered MC's non-obvious memory ownership rules. The most egregious<br>
> example of these is MCObjectStreamer's destructor:<br>
><br>
> MCObjectStreamer::~MCObjectStr<wbr>eamer() {<br>
> delete &Assembler->getBackend();<br>
> delete &Assembler->getEmitter();<br>
> delete &Assembler->getWriter();<br>
> delete Assembler;<br>
> }<br>
><br>
> In the depths of a fever from a head-cold, I snapped. I've been hacking MC<br>
> to convert these raw pointers (and worse: references!) to unique_ptrs<br>
> (apologies to people whose backbends I've broken), but I hit a big blocker<br>
> when I get to Target in "llvm/Support/TargetRegistry.h<wbr>".<br>
><br>
> Target vends MC objects by calling registered ctor functions. E.g.:<br>
><br>
> MCAsmBackend *createMCAsmBackend(const MCRegisterInfo &MRI,<br>
> StringRef TheTriple, StringRef CPU,<br>
> const MCTargetOptions &Options) const {<br>
> if (!MCAsmBackendCtorFn)<br>
> return nullptr;<br>
> return MCAsmBackendCtorFn(*this, MRI, Triple(TheTriple), CPU, Options);<br>
> }<br>
><br>
> The callee owns the resulting object so ideally we would return a<br>
> unique_ptr<MCAsmBackend>, but to do this we'd need access to the definition<br>
> of MCAsmBackend which we can't get without violating layering. (We need the<br>
> definition because unique_ptr<MCAsmBackend> can notionally call<br>
> MCAsmBackend's destructor, though we know it won't here as the whole point<br>
> is to hand ownership back to the caller).<br>
><br>
> There are three approaches I can think of to improve things:<br>
><br>
> (1) Keep the raw pointers, but document *very* clearly, recommending the<br>
> caller stash the result in a unique_ptr immediately.<br>
><br>
> (2) Have the ctor functions take a unique_ptr<T>* as their first argument<br>
> and store the result there (thanks to Dave Blaikie for this suggestion).<br>
> Passing the unique_ptrs by pointer means we don't need a definition for T.<br>
> Usage looks like:<br>
><br>
> std::unique_ptr<MCAsmBackend> MAB;<br>
> Target.createMCAsmBackend(&MAB<wbr>, TheTriple, CPU, Options);<br>
><br>
><br>
> (3) Introduce a 'transient_unique_ptr<T>' type that never calls T's<br>
> destructor, and whose purpose is to pass off ownership to a real unique_ptr:<br>
><br>
> template <typename T><br>
> class transient_unique_ptr {<br>
> public:<br>
> transient_unique_ptr(std::uni<wbr>que_ptr<T> InVal) : Val(InVal.release()) {}<br>
> transient_unique_ptr(transien<wbr>t_unique_ptr<T> &&Other) :<br>
> Val(Other.Val) { Other.Val<br>
> = nullptr; }<br>
> transient_unique_ptr&& operator=(transient_unique_ptr<wbr><T> &&Other) {<br>
> std::swap(Val,<br>
> Other.Val); return *this; }<br>
> ~transient_unique_ptr() { assert(!Val && "value should have been handed<br>
> off"); }<br>
> std::unique_ptr<T> take() { auto Tmp = Val; Val = nullptr; return<br>
> unique_ptr<T>(Val); }<br>
> private:<br>
> T *Val = nullptr;<br>
> };<br>
<br>
</div></div>(4) Move target registration out of Support so that it can include the<br>
required definitions from MC.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>