-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make parented_ptr move only type #13395
base: 2.5
Are you sure you want to change the base?
Conversation
9dbd663
to
f031f51
Compare
769505f
to
82a90b5
Compare
Since parented_ptr is "just" a marker type for an owning pointer (similar to `std::unique_ptr`) it makes sense to enable it to be moved. This makes it possible to use parented_ptr in places where a parented QObject is constructed, but not yet transfered to its owner (this is the case in factory functions).
This serves similar purpose to `std::unique_ptr::reset()` in that it allows the owner to replace destroy the enclosed object indepenent from its parent if needed.
82a90b5
to
fe2c788
Compare
builds cleanly now. please review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have now created a unique_ptr_maybe_parented()
I don't like that idea because it breake the assertion that the object the pointer is pointing to has a parent.
Since parented_ptr is "just" a marker type for an owning pointer
Yes.
(similar to std::unique_ptr
)
No, a std::unique_ptr
holds the ownership of the object the pointer is pointing to. A parented_ptr
is a not owning pointer that verifies that the object is owned by the Qt Object tree. Semantically we can have copies of the pointer as we can copy not owning raw pointers.
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.
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 |
Ah, I see. It has been introduced here: #2172
We should not bend the truth with a misunderstood concept. Let's take the impulse of this PR and fix it.
This was the result of the discussion here: #1161 The idea was to force the user to user a QPointer instead.
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: mixxx/src/widget/findonwebmenuservices/findonwebmenusoundcloud.cpp Lines 34 to 35 in 6b62210
Are perfectly working even before introducing #2172 and are the original use case. The not covered use case is something like this: mixxx/src/widget/wcolorpickeraction.cpp Lines 7 to 20 in 6b62210
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:
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? |
I have (hopefully) cleaned up all cases where we had a parented_ptr without a parent in: 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. |
I think thats fine. some parts of the core-guidelines don't map well to Qts semantic. At least not when you treat
if thats an option yes. I will voice my concerns on your PR that implements the changes. |
Summarizing a bit, apart from the issue that we want the |
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. |
Great. but that leaves the ownership question again since
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
Yeah I think though. Also that case is currently handled by QSharedPointer, right?
Not as far as I can tell since Objects with parented_ptr members are usually QObjects (and thus not copyable) anyways. |
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. |
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. |
You plan to refactor 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. We have even a hand baked QPointer function here: mixxx/src/widget/wwaveformviewer.cpp Line 285 in 6b62210
I have not found the place where the the parent is set. Does it have a parent? |
Yes.
I plan to refactor that as well.
parent of which class? The parent of the waveform widgets gets set here and is always the viewer. mixxx/src/waveform/waveformwidgetfactory.cpp Lines 992 to 998 in 6b62210
|
Got it now. So we have here the creation as parented and here the remote deletion: mixxx/src/waveform/waveformwidgetfactory.cpp Line 478 in 15b4444
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. So we may use parented_ptr inside 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. |
Yes. That was also why I always thought that the internal pointer stored in 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? |
Agreed |
Wait, here some thoughts:
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 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 Proposal:
|
Right, I forgot about that.
I don't know how to make that safe though... |
See individual commit messages for rationale.
TODO: