[Development] The future of smart pointers in Qt API

[Volker Hilsheimer]

> On 1 Feb 2020, at 19:20, Daniel Teske <qt at squorn.de> wrote:
>> Parent-child relationship is not going to go away, it has purpose beyond object lifetime: the visual hierarchy is encoded in the parent/child relationship of widgets, for example.
>> 
>> Making ownership of objects, and assumptions regarding their life-cycle, explicit in code is a good thing though. Using smart pointers is the idiomatic C++ way to do so. But it doens’t have to be the only way; a call to QObject::setParent is IMHO just as explicit, and parent/child in Qt is a concept you have to learn either way.
>> 
>> But sometimes our APIs are messy. QTabWidget::addTab transfers ownership of the QWidget*; QTabWidget::addAction does not transfer ownership of the QAction*. Unless you understand fairly well how QAction works, or unless you read/remember the documentation, it’s impossible to see from the code what happens wrt to object ownership.
>> 
>> API naming is not a scalable way to make the distinction, so finding how we can use smart pointers to make it clear in the code when ownership is transferred is a good idea.
>> 
>> Perhaps the pragmatic approach is to address this in those APIs that take a QObject* but don’t transfer ownership. I believe those are much fewer. Otherwise, the convention can be that any API that takes a raw pointer takes ownership. How this is implemented internally doesn’t matter.
>> 
>> 
> 
> I have already invested quite a bit of time and thinking into how to add unique_ptr apis to Qt. And I encourage everyone to go back and read my previous mails, and/or the actual patch I produced.
> 
> Because I have already presented a design that is:
> 
> * Backwards compatible, existing code continues to work.
> * And adds a lot of memory safety to Qt. Though not without a loop-hole.
> * And actually exists as a experimental patch, not just some abstract idea.
> 
> To explain it once more, the key insight is the following invariant:
> 
> A QObject[1] is either owned by its parent, or by a unique_ptr[2].
> 
> [1]: Some classes use the same parent-child concept as QObject. 
> [2]: Stack allocated objects are essentially equivalent to unique_ptr held ones.
> 
> That implies that a unique_ptr owned QObject should have no parent.
> 
> That invariant ensures that there's exactly one owner for each object, and thus ensures that every object is deleted. 
> 
> But it doesn't guarantee that the owner is what the user expected. 
> 
> So to take your examples:
> 
> QTabWidget::addAction: This never transfers ownership, thus the invariant holds.
> 
> parent->addTab(tab): This has three cases:
> 
> * tab is owned already by parent: Invariant holds
> * tab is owned by a different parent: Invariant holds
> 
> And the last case, and that's the small loop hole in the design:
> * tab has no parent
> 
> That means, the child is owned by a unique_ptr, and the user must have called .get() on it. 
> 
> Thus the small case where the user has to be cautious is when they have a raw pointer for a object owned by a unique_ptr. Luckily with the design I have this is should be rare. 
> 
> In addition to the existing QTabWidget::addTab, the patch then adds:
> 
> QTabWidget::addTab(unique_ptr<>) for which the invariant obviously holds.
> 
> Now to take your next example: setParent is not fine. setParent can be used in 4 different ways:
> 
> child->setParent(child->parent()): Invariant holds
> child->setParent(newParent): Invariant holds
> child->setParent(nullptr): Nope
> orphan->setParent(parent): Nope
> 
> The solution is I have is to replace setParent with two functions: adoptChild + releaseChild, which both ensure that the invariant always holds.
> 
> And to deprecate setParent. I actually made that deprecation opt-in, because of the next part:
> 
> Construction of QObjects:
> 
> The goal is to enable modern C++, meaning that users can choose to never use "new". 
> 
> The first problem is then that a ctor such as this:
> 
> QTabWidget(QWidget *parent = nullptr)
> 
> can be used via make_unique:
> 
> auto child = make_unique<QTabWidget>(someParent);
> 
> That call would then violate the invariant, and thus in my patch I split up the ctor of QTabWidget into 2:
> 
> QTabWidget()
> QTabWidget(QWidget *parent)
> 
> This is obviously backwards compatible. 
> The first ctor can be used without violating the invariant, whereas the second one violates it. And thus it's again optionally deprecated.


Hi Daniel,

I like many things in this proposal, especially the principles; thanks for that!

Where I’m on the fence is the replacing of setParent with “adoptChild”. I think of “parent” as a property of an object, so setParent/parent accessors are the API that fits into my mental model. I have a hard time thinking about “list of children” as that property..

But I think the example below shows exactly the problem <cliff_hanger>

> Now for some classes the parent ptr has additional meaning beyond just being the owner. E.g. for the layout classes. For that reason there's a bit of infrastructure, which makes this work:
> 
> parent->makeChild<QTabWidget>();
> 
> or to take a more complex example:
> 
> parent->makeChild<QLabel>("Hello World!");
> 
> (The infrastructure is a third constructor, and a forwarding function defined via Q_OBJECT)
> 
> For example, converting some random code on https://doc.qt.io/qt-5/qtwidgets-widgets-lineedits-example.html to use makeChild would look like this:
> 
> Window::Window(QWidget
>  *parent)
>     : 
> QWidget
> (parent)
> {
>   QGroupBox *echoGroup = makeChild<QGroupBox>(tr("Echo"));
>   QLabel *echoLabel = 
> echoGroup
> ->makeChild<QLabel>(tr("Mode"));
>   QComboBox *comboBox = 
> echoGroup
> ->makeChild<QComboBox>();
>   [...]
>   QGridLayout *echoLayout = echoGroup->makeChild<QGridLayout>();
> 
>   echoLayout->addWidget(echoLabel, 0, 0);
> 
>   echoLayout
> ->addWidget(echoComboBox, 0, 1);
> 
>   echoLayout
> ->addWidget(echoLineEdit, 1, 0, 1, 2);
> 


This works, but now you are encoding the visual hierarchy of the widgets in two places: when asking the presumed parent to create the widgets; and when setting up the layout, which might do things differently (esp once you start refactoring complex UIs).



> Or if using unique_ptrs:
> 
> Window::Window(QWidget
>  *parent)
>     : 
> QWidget
> (parent)
> {
>   auto echoGroup = std::make_unique<QGroupBox>(tr("Echo"));
>   auto echoLabel = std::make_unique<QLabel>(tr("Mode"));
>   auto comboBox = std::make_unique<QComboBox>();
>   [...]
>   auto echoLayout = std::make_unique<QGridLayout>();
> 
>   echoLayout->addWidget(std::move(echoLabel), 0, 0);
> 
>   echoLayout
> ->addWidget(std::move(echoComboBox), 0, 1);
> 
>   echoLayout
> ->addWidget(std::move(echoLineEdit), 1, 0, 1, 2);


I don’t think these lines above are correct.

QBoxLayout::addWidget does *not* take ownership. Widgets are reparented to their correct parent once you call QWidget::setLayout in the next line, and QLayout operates on QLayoutItems that hold a weak pointer to the widgets that the layout places.


>   echoGroup
> ->setLayout(std::move(echoLayout));
> And obviously the use can choose whatever they like, though I would recommend the first variant. 
> 
> Imho this is the nicest aspect of this design. Adding lots of unique_ptr apis lead to no usage of unique_ptrs. Instead, there's just a clearer way to construct the usual parent-child relationships.
> 
> There are some details I'm omitting here, and some apis that needs more work but that's the core idea.



So, how do we get a weak “observer" pointer into the layout, and reset the unique_ptrs only in the setLayout line when the widgets are de-facto no longer owned by the unique_ptr?


Cheers,
Volker