[Development] The future of smart pointers in Qt API

[Volker Hilsheimer]

> On 3 Feb 2020, at 19:09, Daniel Teske <qt at squorn.de> wrote:
> 
> 
>> 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..
> 
> Well this should not compile. (Or rather be optionally deprecated.)
> 
> unique_ptr<> a;
> a->setParent(b);


That’s my point. Parent is a property of an object. Properties in Qt are set and retrieved via accessors that follow a very consistent naming convention.

And calling setParent is just as explicit in terms of ownership as calling std::make_unique or QObject::makeChild.


>> 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).#
> 
> That's not a problem. The code simply behaves as if you would have used the current constructors that take a parent parameter.
> 
> Ownership transfers between different qt parents don't violate the invariant. And they continue to work just like before.


So how is the above usage of makeChild an improvement over calling operator new to instantiate your objects?

It leads to confusing code if you make a child object via a parent that - after the layout kicks in - is actually no longer the parent (which is the case when you refactor a complex UI, and don’t remember to change both the code that instantiates the objects, and sets the layout up)?


>>> 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.
> 
> It's even more complicated than that, if the layout is already set on a widget a call to QBoxLayout::addWidget does transfer ownership.


Ah, but not to the layout, which the code makes you believe it does. The new owner of echoLabel is “this”, not echoLayout.


> But for a parent-less QBoxLayout::addWidget(QWidget *) does not take ownership. So whether a call to addWidget does transfer ownership is really hard to tell.
> 
> Which I personally find surprising and I actually missed that in my patch and Giuseppe had to point it out.
> 
> But that just means that QBoxLayout::addWidget(std::unique_ptr<>) has to behave imho saner in that edge case and actually delete the widgets that were added but never parented. (And I should really implement that to show how it would work.)


That seems like a terrible idea. One of the layout’s jobs is to set up the parent/child hierarchy so that you as a developer don’t have to.

Do we really want to make it harder for people to write UIs just because the resulting code doesn’t satisfy a C++ idiom?


> There's another pain point in the patch currently, and that has to do with the overloaded meaning of the parent ptr for QDialog. For QDialog in addition to the memory ownership that also controls modality. The other cases of overloaded meaning that I found in qtbase are as far as I can tell fine.
> 
> Meaning a line such as this:
> 
> QDialog dialog(parent);
> 
> is technically a doubly owned object, because it's both owned by the stack and by the passed parent. And that same problem makes it hard to make this memory safe.
> 
> The solution to that, is to fix Qt here. Modality and parent should not be coupled in this way.


It’s a solution to a problem that people don’t seem to have though. At least I don’t recall this being a significant source of confusion during my 9 years at Qt support. Yes, sometimes people allocate an object on the stack and then wonder why the widget doesn’t show up in their UI. This is not exactly a subtle issue that only sometimes causes unexplicable undefined behavior. Ditto for modality or window stacking.

The parent/child model in Qt has several implications which I think (and it’s ok to disagree) are consistent, given the purpose of QObject (and more so QWidget):

* ability to respond to events within a chain of responders
* visual hierarchy, including stacking order for top-level widgets
* child lifetime coupled to parent's
* meta-object based stuff

If you don’t care about the things that QObject/QWidget does for you, then don’t use QObject/QWidget. And if you do use QObject/QWidget and get a consistent object model from it, then I’m really not convinced that adding smart pointers to the API is making the code clearer, or in any way more explicit. It’s not like the ability of using std::unique_ptr means that people no longer have to be familiar with Qt’s basic concepts to use it correctly.

Exceptions as mentioned are those APIs that look like they take ownership, but don’t (such as QTabWidget::addAction).


Volker