I tried to gather all the shared pointers ideas we discussed with @gladhorn during his refactorings.
Points 1 and 2 are not yet implemented, but would be nice to have.
Point 3 is a real problem we have in Krita (see bug 372184). I don't know how it can be solved in a general way. Any ideas are welcome!
| No Linters Available |
| No Unit Test Coverage |
| HACKING | ||
|---|---|---|
| 132 | My take on this is: When an object is owned by several places, use QSharedPointer. Objects that own the shared pointer, have it as member. For objects that don't own an object, but refer to it, use QWeakPointer. When then using the object, it must be checked for null. QWeakPointer<Foo> myWeakReference = shared.toWeakRef();
...
// usage:
QSharedPointer<Foo> foo = myWeakReference.toStrongRef();
if (foo) {
foo->doStuff();
} | |
| 135 | I don't see the advantage of this? I would try to make it as little magic as possible and actually like seeing the type because it's easiest to read for new people. Can you give an example where the proposal helps? QSharePointer<Foo> myFooPointer(new Foo()); | |
| 140 | This makes adding explicit impossible. I'd like to see: auto myNullPtr = KisSharedPtr<Foo>(nullptr) | |
| 145 | I think for parent child relations: parent owns child, if child needs a pointer to parent or sibling, it should use weak pointers. Keeping pointers to siblings doesn't sound all that great though, if the nodes have pointers to their parent, getting the sibling is easy and means less book-keeping. This can be hidden in nice API. The only reason I see for keeping sibling pointers would be in performance critical code paths as optimization. | |
Ok, it seems like I need to change the wording in several places... So I'll mark it as WorkIsPlanned
| HACKING | ||
|---|---|---|
| 135 | I think my wording is just incorrect. Of course, if possible, one should use explicit c-tor if it is already present. But there are at least two cases when specifying is a cumbersome:
KisPSDLayerStyleSP KisPSDLayerStyle::clone() const
{
// return type is deduced automatically
return toQShared(new KisPSDLayerStyle(*this));
}
KisViewManagerPrivate(KisViewManager *_q, QWidget *_q_parent)
{
...
canvasResourceManager.addDerivedResourceConverter(toQShared(new KisCompositeOpResourceConverter));
canvasResourceManager.addDerivedResourceConverter(toQShared(new KisEffectiveCompositeOpResourceConverter));
canvasResourceManager.addDerivedResourceConverter(toQShared(new KisOpacityResourceConverter));
canvasResourceManager.addDerivedResourceConverter(toQShared(new KisFlowResourceConverter));
canvasResourceManager.addDerivedResourceConverter(toQShared(new KisSizeResourceConverter));
canvasResourceManager.addDerivedResourceConverter(toQShared(new KisLodAvailabilityResourceConverter));
canvasResourceManager.addDerivedResourceConverter(toQShared(new KisEraserModeResourceConverter));
canvasResourceManager.addResourceUpdateMediator(toQShared(new KisPresetUpdateMediator));
...
}The main benefit of such approach is that when you convert raw-pointer/kis-shared-pointer code into the strict pointers policy (like in QSharedPointer), the refactoring becomes a *lot* simpler. You just put toQShared() and the compiles does all the deduction for you. I once tried to convert KisPaintOpSettings SP into QSharedPointer and toQShared() was a really great helper. Basically it can help with fixing the warnings produced by your patch D3307. And yes, in the example you provided, explicit constructor is preferred: QSharePointer<Foo> myFooPointer(new Foo()); | |
| 140 | Well, such usage of auto is not recommended by other paragraphs of this file :) ''auto'' is recommended for iterators/loops only. And speaking truly I don't see why this code is impossible. You mean it is forbidden by the policy of the C++ standard? Anyway, it is again a fault in my wording. I didn't mean an explicit construction. I meant returns from a function and default parameters.
Right now we have this code: KisNodeSP KisNode::firstChild() const
{
QReadLocker l(&m_d->nodeSubgraphLock);
return !m_d->nodes.isEmpty() ? m_d->nodes.first() : 0;
}And I propose to refactor it into: KisNodeSP KisNode::firstChild() const
{
QReadLocker l(&m_d->nodeSubgraphLock);
return !m_d->nodes.isEmpty() ? m_d->nodes.first() : nullptr;
}
Code now: KisPaintDevice(KisNodeWSP parent, const KoColorSpace * colorSpace, KisDefaultBoundsBaseSP defaultBounds = 0, const QString& name = QString()); Code after refactoring: KisPaintDevice(KisNodeWSP parent, const KoColorSpace * colorSpace, KisDefaultBoundsBaseSP defaultBounds = nullptr, const QString& name = QString()); Again, it'll fix a lot of warnings in D3307 and will make your like a lot easier :) | |
| 145 | You described exactly the case we have here. The problem is that calling any hierarchy traversal code from the c-tor or d-tor destructs the object. Because any traversal involves promotion of a weak pointer into a real pointer that points to a not-initialized/already-destructed object. I have no clear idea now to fix that :( | |
18:57 < fregl> dmitryK|log: btw, on a completely unrelated note: prefer qobject_cast over dynamic_cast for qobjects, it's cheaper :)