> On 1 Feb 2020, at 19:20, Daniel Teske <[email protected]> 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
_______________________________________________
Development mailing list
[email protected]
https://lists.qt-project.org/listinfo/development
