[Lldb-commits] [PATCH] D58946: [SBAPI] Don't check IsValid in constructor

[Pavel Labath via lldb-commits]

On 05/03/2019 17:19, Jonas Devlieghere wrote:
> Sounds good, I’ll make this change before landing, unless you have 
> further comments?
> 
> On Tue, Mar 5, 2019 at 4:29 AM Pavel Labath via Phabricator 
> <reviews at reviews.llvm.org <mailto:reviews at reviews.llvm.org>> wrote:
> 
>     labath added a comment.
> 
>     The copy constructors and assignment operators are very repetitive.
>     What would you say to a function like
> 
>        template<typename T>
>        void clone(std::unique_ptr<T> &dest, const std::unique_ptr<T> &src) {
>        if (&dest == &src)
>          return;
>        if (src)
>          dest = llvm::make_unique<T>(*src);
>        else
>          dest.reset();
>        }
> 
>     (and a similar one for shared_ptrs)? Then both copy constructors and
>     assignment operators could be just implemented as `clone(m_whatever,
>     rhs.m_whatever);`. (Assuming we don't care about pessimizing the
>     self-assignment case this could even be `m_whatever =
>     clone(rhs.m_whatever);`)
> 
> 
>     Repository:
>        rLLDB LLDB
> 
>     CHANGES SINCE LAST ACTION
>     https://reviews.llvm.org/D58946/new/
> 
>     https://reviews.llvm.org/D58946
> 
> 

Yeah, that's fine. The only thing I could possibly add is that this 
approach depends on not "mirroring" the default constructor, which is 
the thing you were trying to the in the original patch, and I don't 
think it's right or necessary.
It's not right because I think a copy constructor should make a exact 
copy, regardless of what the default constructor of the same object 
would create.
And it's not necessary because if the default constructor already 
initializes the unique_ptr member (which is the thing you are trying to 
mirror), then you should not ever encounter a object with an empty 
unique_ptr to copy from.

pl