make parented_ptr move only type

[Swiftb0y]

See individual commit messages for rationale. TODO:

• [x] avoid memory leak when object being managed is valid but has no parent.

[Swiftb0y]

builds cleanly now. please review.

[Swiftb0y]

maybe_parented

That assumption was already in parented_ptr previously (as the comment states to allow for the usecase where the parented_ptr is created but the parent is set later). I only made the check for it present at all times so we don't leak memory when the assumption is violated.

A parented_ptr is a not owning pointer that verifies that the object is owned by the Qt Object tree.

I don't think thats the case. parented_ptr is not copyable so it was never supposed to denote shared ownership IMO. From what I could understand by its usage is that it effectively is supposed to be the owner of the managed QObject (even if technically the parent is the owner).

So whats the semantic you propose instead? Keeping raw owning pointers is not an option because they don't express that ownership (in fact the core-guidelines says raw pointers should always be assumed to be non-owning).

The entire motivation for this was a not-yet-published branch that cleans up some of the recent waveform code where I needed this functionality (I needed an owning handle to a parented QObject so I could delete it independent from the parent, this is also the motivation for the new parented_ptr::reset()), so please suggest alternatives.

[daschuer]

That assumption was already in parented_ptr previously (as the comment states to allow for the usecase where the parented_ptr is created but the parent is set later).

Ah, I see. It has been introduced here: https://github.com/mixxxdj/mixxx/pull/2172 Reading the related discussion now, I think it did not "fix" the cumbersome recommendation to use a std::unique_ptr() it just weakens the original power of make_parented()

even if technically the parent is the owner

We should not bend the truth with a misunderstood concept. Let's take the impulse of this PR and fix it.

I don't think thats the case. parented_ptr is not copyable so it was never supposed to denote shared ownership IMO.

This was the result of the discussion here: https://github.com/mixxxdj/mixxx/pull/1161 The idea was to force the user to user a QPointer instead.

So whats the semantic you propose instead? Keeping raw owning pointers is not an option because they don't express that ownership (in fact the core-guidelines says raw pointers should always be assumed to be non-owning).

That is fully correct. The parented_ptr is a non-owning raw pointer with just an assertion that it is owned by the QT object tree.

Things like: https://github.com/mixxxdj/mixxx/blob/6b62210eed71869fb2dd39c12261328912caf528/src/widget/findonwebmenuservices/findonwebmenusoundcloud.cpp#L34-L35

Are perfectly working even before introducing #2172 and are the original use case. The not covered use case is something like this:

https://github.com/mixxxdj/mixxx/blob/6b62210eed71869fb2dd39c12261328912caf528/src/widget/wcolorpickeraction.cpp#L7-L20

Here we have the issue that the owning widget tree does not exists when the QWidgetAction is created.

But we can easily swap the initialization order and introduce more smart pointer:

WColorPickerAction::WColorPickerAction(WColorPicker::Options options, const ColorPalette& palette, QWidget* parent)
        : QWidgetAction(parent) {
    auto pWidget = std::make_unique<QWidget>();
    auto pLayout = make_parented<QHBoxLayout>(pWidget.get());
    pWidget->setLayout(pLayout);
    pWidget->setSizePolicy(QSizePolicy());
    m_pColorPicker = make_parented<WColorPicker>(options, palette, pWidget.get());
    pLayout->addWidget(m_pColorPicker);
    pLayout->setSizeConstraint(QLayout::SetFixedSize);
    setDefaultWidget(pWidget.release());
    connect(m_pColorPicker.get(), &WColorPicker::colorPicked, this, &WColorPickerAction::colorPicked);
}

Which is finally a way better solution with a tight memory management.

There is a remaining weakness that m_pColorPicker is a non-owning pointer stored as member variable which can be seen as a core-guideline violation. We may use a QPointer for storage, but the overhead and required null checks are not required because it is unlikely that the widget tree is destroyed before the WColorPickerAction() object.

How do you think about that issue?

By the way, this seems to be the only place where our concerns apply. Instead to allow more of these situations I would prefer to move the parent assertion back into the constructor and apply the proposed fix.

What do you think?

[daschuer]

I have (hopefully) cleaned up all cases where we had a parented_ptr without a parent in: https://github.com/mixxxdj/mixxx/pull/13411 So the most important issue is now solved IMHO.

I can confirm the issue that using parented_ptr can be tricky, because it does not allow to follow Qt examples literally. The approach presented here is more easy to use, but has some caveats.

[Swiftb0y]

There is a remaining weakness that m_pColorPicker is a non-owning pointer stored as member variable which can be seen as a core-guideline violation. [...]

How do you think about that issue?

I think thats fine. some parts of the core-guidelines don't map well to Qts semantic. At least not when you treat parented_ptr as non-owning. Safety-wise its completely fine because the member variable will go out of scope before the parent QObject destructor is run-

By the way, this seems to be the only place where our concerns apply. Instead to allow more of these situations I would prefer to move the parent assertion back into the constructor and apply the proposed fix.

What do you think?

if thats an option yes. I will voice my concerns on your PR that implements the changes.

[Swiftb0y]

Summarizing a bit, apart from the issue that we want the parented_ptr to always be constructed with an parent, are you okay with making parented_ptr itself movable? What about the reset() function (or rather its motivating usecase)?

[daschuer]

are you okay with making parented_ptr itself movable?

In general there is nothing against making it movable or add a reset() function.

I just don't see a uses case, do you have something specific in mind. We already have the get() function to pass it around as raw pointer. A move would be more expensive due to zeroing out the original object.

Alternatively we may consider to copy it. This would allow a function to take a parented_ptr enforcing that it is parented. Probably a stupid idea?

Along this topic we may consider what happens if we copy an object with a parented_ptr member. This is currently not possible. Is there a reason for it?

[Swiftb0y]

In general there is nothing against making it movable or add a reset() function.

Great. but that leaves the ownership question again since reset() would delete m_ptr, making ownership confusing again.

A move would be more expensive due to zeroing out the original object.

only in the case where move-elision or named return value optimization fails. The cases where I need the move is essentially a factory function so either of those two would probably also apply. Btw gcc added -Wnrvo recently helping to diagnose cases where NRVO is not applied even it could in theory.

Probably a stupid idea?

Yeah I think though. Also that case is currently handled by QSharedPointer, right?

Along this topic we may consider what happens if we copy an object with a parented_ptr member. This is currently not possible. Is there a reason for it?

Not as far as I can tell since Objects with parented_ptr members are usually QObjects (and thus not copyable) anyways.

[daschuer]

In general there is nothing against making it movable or add a reset() function.

Great. but that leaves the ownership question again since reset() would delete m_ptr, making ownership confusing again.

We must not delete the object of a parented_ptr. This works, but comes with extra cost to clean up the Qt object tree. So this reset() will only reset the pointer without a deleting it.

[Swiftb0y]

right, but that's not the point. The purpose is specifically to cut the live of the QObject short and the cost of the deletion is intended (not in general on destruction, just for the reset). The usecase here is that the waveforms are currently implemented like that and in order to change from a displayed waveform to another, we have to delete the object independent from its viewer (which is also the parent). This is especially noticeable for the empty waveform which is modeled as no waveform object at all. If the object waveform object is not deleted, the waveform viewer will still display it. I have no other way achieve this without calling the destructor, I'm happy to apply an alternative if you can think of one.

[daschuer]

You plan to refactor WaveformWidgetFactory::createWaveformWidget()?

I think we have there a bunch of other "problems" to solve because the QWidget handled by different classes. It is a member of WaveformWidgetAbstract and WWaveformViewer. So we have here a kind of shared ownership between. QPointer() is here the correct type to express that.

We have even a hand baked QPointer function here: https://github.com/mixxxdj/mixxx/blob/6b62210eed71869fb2dd39c12261328912caf528/src/widget/wwaveformviewer.cpp#L285

I have not found the place where the the parent is set. Does it have a parent?

[Swiftb0y]

You plan to refactor WaveformWidgetFactory::createWaveformWidget()?

Yes.

I think we have there a bunch of other "problems" to solve because the QWidget handled by different classes. It is a member of WaveformWidgetAbstract and WWaveformViewer. So we have here a kind of shared ownership between.

I plan to refactor that as well.

I have not found the place where the the parent is set. Does it have a parent?

parent of which class? The parent of the waveform widgets gets set here and is always the viewer.

https://github.com/mixxxdj/mixxx/blob/6b62210eed71869fb2dd39c12261328912caf528/src/waveform/waveformwidgetfactory.cpp#L992-L998

[daschuer]

Got it now. So we have here the creation as parented and here the remote deletion:
https://github.com/mixxxdj/mixxx/blob/15b4444584b9cd2514946d6a645746cae3f5a315/src/waveform/waveformwidgetfactory.cpp#L478 which is also a bit cumbersome, since it is unclear who is the owner here.

Some thoughts:

We may argue that the Qt is not the owner semantically however it is as side effect when populating the widget tree. Since QT works like that like that we need to deal with it.

So we may use parented_ptr inside createAllshaderWaveformWidget() and then pass the pointer as QPointer (toWeakRef()) around. When we want to delete the widget we can use pWidgetQPointer->deleteLater() which remove the Widget the safest way form the widget stack and automatically zeros out all QPointer copies.

I agreed there is a demand for a secondary owner concept here. We have the issue how to avoid that a viewer does not end up with two WaveformWidget Here the tracing nature of unique_ptr() which is not so much unique would be helpful. Maybe we can extend a QPointerHolder ("Widget tree porxy" / "QPointerUnique" other fancy name that indicates that it manages the ownership) member of the viewer that does this.

For my understanding your is is it to integrate it in the parented_ptr(). I am not strictly against it, I am only worrying that it might has overhead or confusion in all other places where this "forwarded ownership" is not an issue.

[Swiftb0y]

We may argue that the Qt is not the owner semantically however it is as side effect when populating the widget tree.

Yes. That was also why I always thought that the internal pointer stored in parented_ptr was actually the owner (at least semantically, even if not not technically).

I'm not sure if there really is need for this secondary owner concept. If we instead view the parented_ptr as if it actually was the owner, then the mental model simplifies and a reset method would make sense.

Anyways, before wasting time going down this path, I would like to explore the problem again on the refactor branch (after all WaveformWidgetFactory is wayy too big and cluttered anyways). Can we agree on only adding move-ability for now and everything else can be done in the PR where I need it?

[daschuer]

Agreed

[daschuer]

Wait, here some thoughts:

If we instead view the parented_ptr as if it actually was the owner, then the mental model simplifies and a reset method would make sense.

This works for me, but moving the parented_ptr around would undermine this concept, since in that case the parented_ptr needs to be always a member variable and must not moved away from the owning object.

Currently we use the parented_ptr like this m_pObject = make_parented<QObject>(this) where the proposed mental model applies and pObject = make_parented<QObject>(pParent) where it does not apply.

The first one (member) must not be copied nor moved the later (local var) can copied and moved freely.

The factory we have creates local var type and it is copied later into the member type. Ideally the factory also receives a pointer to parented_ptr<QObject> m_pObject and stores the pointer directly at the destination. This can than also deleteLater() an previously stored Object.

Proposal:

• add copy and move to parented_ptr
• add a new one that is used as a member variable only and is not movable and copyable and deletes the pointer on reset()

[Swiftb0y]

This works for me, but moving the parented_ptr around would undermine this concept, since in that case the parented_ptr needs to be always a member variable and must not moved away from the owning object.

Right, I forgot about that.

add a new one that is used as a member variable only and is not movable and copyable and deletes the pointer on reset()

I don't know how to make that safe though...