<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Mar 5, 2019 at 8:41 AM Pavel Labath <<a href="mailto:pavel@labath.sk">pavel@labath.sk</a>> wrote:<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">On 05/03/2019 17:19, Jonas Devlieghere wrote:<br>
> Sounds good, I’ll make this change before landing, unless you have <br>
> further comments?<br>
> <br>
> On Tue, Mar 5, 2019 at 4:29 AM Pavel Labath via Phabricator <br>
> <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a> <mailto:<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>>> wrote:<br>
> <br>
>     labath added a comment.<br>
> <br>
>     The copy constructors and assignment operators are very repetitive.<br>
>     What would you say to a function like<br>
> <br>
>        template<typename T><br>
>        void clone(std::unique_ptr<T> &dest, const std::unique_ptr<T> &src) {<br>
>        if (&dest == &src)<br>
>          return;<br>
>        if (src)<br>
>          dest = llvm::make_unique<T>(*src);<br>
>        else<br>
>          dest.reset();<br>
>        }<br>
> <br>
>     (and a similar one for shared_ptrs)? Then both copy constructors and<br>
>     assignment operators could be just implemented as `clone(m_whatever,<br>
>     rhs.m_whatever);`. (Assuming we don't care about pessimizing the<br>
>     self-assignment case this could even be `m_whatever =<br>
>     clone(rhs.m_whatever);`)<br>
> <br>
> <br>
>     Repository:<br>
>        rLLDB LLDB<br>
> <br>
>     CHANGES SINCE LAST ACTION<br>
>     <a href="https://reviews.llvm.org/D58946/new/" rel="noreferrer" target="_blank">https://reviews.llvm.org/D58946/new/</a><br>
> <br>
>     <a href="https://reviews.llvm.org/D58946" rel="noreferrer" target="_blank">https://reviews.llvm.org/D58946</a><br>
> <br>
> <br>
<br>
Yeah, that's fine. The only thing I could possibly add is that this <br>
approach depends on not "mirroring" the default constructor, which is <br>
the thing you were trying to the in the original patch, and I don't <br>
think it's right or necessary.<br></blockquote><div><br></div><div>I thought the same yesterday. The difference is the result of always being able to create a default object form Python. However, not all SB objects have a default construct. Some things are not valid unless they are returned by a factory. That's the reason that some objects always initialize their opaque pointer, and some don't. The mirroring of the default constructor is to retrain this semantic in the copy constructor. </div><div> </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">
It's not right because I think a copy constructor should make a exact <br>
copy, regardless of what the default constructor of the same object <br>
would create.<br></blockquote><div><br></div><div>I think we agree, but just to be sure: an exact copy of what? The lldb object or the lldb_private object?  </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">
And it's not necessary because if the default constructor already <br>
initializes the unique_ptr member (which is the thing you are trying to <br>
mirror), then you should not ever encounter a object with an empty <br>
unique_ptr to copy from.<br></blockquote><div><br></div><div>Yes, if all code paths initialize the opaque pointer and nothing every unsets it, then this is true. Then we can also remove all the spurious if(opaque_ptr) checks. But what about classes that don't initialize their unique pointer, they still need to be copyable. </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">
<br>
pl<br>
</blockquote></div></div>