On Tue, Mar 5, 2019 at 8:41 AM Pavel Labath <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>> 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 >
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
