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

[Pavel Labath via lldb-commits]

On 05/03/2019 17:58, Jonas Devlieghere wrote:
> 
> 
> On Tue, Mar 5, 2019 at 8:41 AM Pavel Labath <pavel at labath.sk 
> <mailto:pavel at labath.sk>> wrote:
> 
>     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>
>     <mailto: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.
> 
> 
> 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.
> 
>     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.
> 
> 
> I think we agree, but just to be sure: an exact copy of what? The lldb 
> object or the lldb_private object?

Whatever is the semantics we want. In the cases that we're talking about 
here, it's the lldb_private object.

> 
>     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.
> 
> 
> 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.

That's fine, and in that case you copy the empty pointer as an empty 
pointer. My point is the copy operation should be defined in terms of 
what is the state of the source object, not in terms of what the default 
constructor does. The default constructor is just one way to initialize 
the object, but it's possible that the unique_ptr became uninitialized 
in a different way too.

pl