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

[Jonas Devlieghere via lldb-commits]

On Tue, Mar 5, 2019 at 8:41 AM Pavel Labath <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>> 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?


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


>
> pl
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20190305/fda632a6/attachment.html>