[Development] unique_ptr and Qt

[Daniel Teske]

Hi,

I've done some research into how supporting unique_ptr in Qt might look 
like.

I have a patch which adds apis using unique_ptr for ownership transfer 
to almost all of QtBase and a patch for Qt Creator to show how using 
those apis looks like.

Qt: https://codereview.qt-project.org/231543
Qt Creator: https://codereview.qt-project.org/231545

It's nowhere finished, and I have no expectations of it going into Qt in 
the near future, nor the intention to finish the patch. The intent was, 
to have some fun, to explore the space of what is possible, to show how 
it could look like and to maybe start a discussion on what Qt can and 
should do in that direction. Also I run out of time to properly polish it.

This patch is backwards compatible, existing code continues to work 
exactly the same and has a very high degree of source compatibility. It 
allows users to opt-in to warnings that indicate unclear ownership 
semantics and allows for piece by piece porting of code. It requires 
C++17 though. And unfortunately due to a bug MSVC 15.7 is not good 
enough. That bug in guaranteed copy elision is supposed to be fixed in 15.8.

The additional apis are enabled via a configure flag and are inline 
functions.

The core idea is this invariant:

A Qt Object either is owned by a unique_ptr, is owned by a parent, or is 
allocated on the stack. (A stack allocated object, might or might not 
have a parent.)

Thus the first addition are 3 new functions for creating objects, all 
very similar to std::make_unique.

std::unique_ptr<T> qmakeUnique<T>(Args ...);
T* QObject::makeChild(Args ...);
T makeOnStack(ParentType, Args ...);

Those functions ensure that the returned object observe the invariant. 
That is, qmakeUnique passes a nullptr as the parent pointer to T.  And 
widget->makeChild(...) passes widget as the parent pointer. Thus if the 
constructor uses the parent pointer to set it's parent, the invariant 
holds and is ensured at compile time. Some types, e.g. QEvent, 
QTreeWidgetItem are treated a bit differently, see the patch for details.

I'll discuss various classes of functions:

* Functions "taking" ownership, e.g. QLayout::addWidget( child )

Note: Qt functions that "take" ownership, don't actually always take 
ownership. If the passed in argument is already owned by the right 
parent, ownership is not transferred.

To ensure that the invariant holds, there are two cases to consider:

a) The child is owned by a parent. => Use the addWidget(QWidget 
*rawPointer) overload
b) The child is owned by a unique_ptr => Use the new 
addWidget(std::unique_ptr<QWidget> widget) overload

For example this code is then possible:

auto label = qmakeUnique("label text");
layout->addWidget(std::move(label));

A undiagnosed and potential pitfall, is this code:

auto label = qmakeUnique("label text");
layout->addWidget(label.get());

which leads to double deletion. Calling a qt function that takes 
ownership with a raw pointer derived from a unique_ptr is a user bug. 
While obvious in this case, that isn't always so.

* Functions releasing ownership / creating new objects

There aren't actually not a lot of functions in this category. For 
example QLayout::removeWidget() does not release ownership of the 
widget, instead ownership is transferred if the widget is added to a 
different layout.

For functions that actually do release ownership / create objects, add a 
function returning a unique_ptr mostly named either "makeXXX" or 
"releaseXXX".

So how do I prevent users from calling the old function?

a) Not at all, this makes the code fully backwards compatible.
b) The user can opt into deprecation warnings by defining: 
QT_DEPRECATED_UNSAFE_API For Qt Creator patch, I defined that only in 
the coreplugin plugin.

* Other points
I probably missed quite a few special cases, since my aim was to cover 
as much as qtbase as possible, without spending too much time on each 
individual class. But for the most part, there weren't that many special 
cases. I left a few "TODO"s in the patch for apis that would take more 
work. I don't think there was anything completely unsolvable, but quite 
a few pain points.

Having unique_ptr as a vocabulary type also has some other benefits:

Some apis in Qt would really love to have a way to ensure that the 
caller takes care of deleting the resource. But because there is no way 
in C++98 to ensure that, they don't transfer ownership. For example, the 
documentation of QTcpServer::nextPendingConnection() is:

 > The socket is created as a child of the server, which means that it 
is automatically deleted when the QTcpServer 
<https://doc.qt.io/qt-5/qtcpserver.html> object is destroyed.
 > It is still a good idea to delete the object explicitly when you are 
done with it, to avoid wasting memory.

With unique_ptr there would be a way to ensure that the caller takes 
ownership by returning the QTcpSocket wrapped in a unique_ptr.

Also while writing this patch, I noticed several functions whose 
ownership semantics are too strange for my taste and should arguably be 
changed even without adding unique_ptr. For example: void 
QGraphicsGridLayout::removeAt(int index) transfers ownership of a item 
that is neither a parameter nor a return value.

I also ported a very small part of Creator to use those new apis and I 
actually found only one place that leaks memory. Some of the code looks 
arguably better, imho makeChild does make the code easier to read. In 
other parts, Creator has fancy memory ownership semantics, which are not 
easily representable with unique_ptr's. And unique_ptr can be bit 
awkward to use.

And as a last though, having unique_ptr as part of the vocabulary types 
also allows for better apis:

Consider QScrollArea and its functions: void setWidget(QWidget *w); void 
setCornerWidget(QWidget *w);

Even though they have the exact same signature, and very similar names, 
their behaviour is different in how they treat the old replaced widget.

setWidget() deletes the old widget, there is afaik no way to reuse a 
widget once it has been set via setWidget.

On the other hand, setCornerWidget() merely hides the old widget. (I 
think the use case here is switching between multiple corner widgets.)

With unique_ptr, a imho better api would be:

unique_ptr<QWidget *> replaceWidget/replaceCornerWidget(QWidget *)

If the user does not care about the old widget, just ignoring the return 
value gets the old widget deleted. If the user wants to reuse the widget 
somewhere, s/he has ownership of it.

* Short guide to patch reading:

Since the patches are pretty big and most of it is pretty uninteresting, 
for reading it I suggest:

1) Look at the changes in qglobal.h.
2) Look at the qwidget.h
3) Look at qtreewidgetitem.h
4) Look at the creator patch at a random file
5) Look at TODO comments which explain some of the remaining problems

* My Conclusion

Adding unique_ptr supporting apis is possible. (Though clearly this 
patch would need a lot of refining.) But, it adds a lot of complexity, 
and can be awkward to use. On the other hand, as more users become 
accustomed with using unique_ptr in their code, the demand for Qt to 
support them will grow. So imho research in that direction must happen.

Since the mail is already pretty long, I'll stop here. If you have 
questions or suggestions, due to some technical difficulties, I can't 
respond until Friday.

I'll be at the Contributor Summit, so if there's sufficient interest I 
can add a session to the schedule.

daniel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.qt-project.org/pipermail/development/attachments/20180605/512bcc38/attachment.html>