On 05/03/2019 17:58, Jonas Devlieghere wrote:
On Tue, Mar 5, 2019 at 8:41 AM Pavel Labath <pa...@labath.sk
<mailto:pa...@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
> <revi...@reviews.llvm.org <mailto:revi...@reviews.llvm.org>
<mailto:revi...@reviews.llvm.org <mailto:revi...@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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
