3
\$\begingroup\$

I am fully aware that Qt is quite good with handling ownership in its object trees. The usual pattern seems to be to use a raw new, assigning the result to a raw pointer and then giving up the ownership with some widget->add(...) call.

This does not seem C++11/C++14, or modern, or RAII-style to me -- especially not in the light of the new C++ Core guidelines and the Guidelines Support library (GSL).

I started using unique_ptr to be on the safe side during set-up. The widget->add(...) then gets the unique_ptr::release()-result. So if anything goes awry it is still sure there is no leak. But because after release() the content of the unique_ptr is nullptr I stored its content non-owning (an "alias") in a separate variable for later use (like conntect and other method calls).

That looks quite ok to me in simple cases. But when I designed my first own Widget class I had a lot of boiler-plate code around all this unique_ptr/non-owning-alias.

Here is my header file:

// 005-my-window/my_window.hpp
#ifndef MY_WINDOW_HPP
#define MY_WINDOW_HPP
#include <QWidget>
#include <QVBoxLayout>
#include <QPushButton>
#include <QLabel>
#include <QObject>
class MyWindow : public QWidget {
    Q_OBJECT
public:
    MyWindow(QWidget* parent = nullptr);
private: // nicht selbst Besitzer dieser QWidgets:
    QLabel* label_ = nullptr;
    QPushButton* button0_ = nullptr;
    QPushButton* button1_ = nullptr;
    QVBoxLayout* layout_ = nullptr;
    int cnt_ = 0; // zählt die Klicks mit
private slots:
    void setText();
};
#endif // MY_WINDOW_HPP

And here the implementation:

// 005-my-window/my_window.cpp
#include "my_window.hpp"
#include <QApplication>
#include <memory>
MyWindow::MyWindow(QWidget *parent)
    : QWidget{parent}
{
    auto label = std::make_unique<QLabel>("old text");
    label_ = label.get();
    auto button0 = std::make_unique<QPushButton>("Increment");
    button0_ = button0.get();
    auto button1 = std::make_unique<QPushButton>("Quit");
    button1_ = button1.get();
    auto layout = std::make_unique<QVBoxLayout>(this);
    layout_ = layout.get();
    layout->addWidget(button0.release());
    layout->addWidget(button1.release());
    layout->addWidget(label.release());
    setLayout(layout.release());
    connect( button0_, SIGNAL( clicked() ), this, SLOT( setText() ) );
    connect( button1_, SIGNAL( clicked() ), qApp, SLOT( quit() ) );
}
void MyWindow::setText() {
    QString msg{"Label updated "};
    msg.append(QString{}.setNum(++cnt_));
    msg.append(" times");
    label_->setText(msg);
}

Well, I need the non-owning raw pointer members to the QWidgets for later use, obviously. But in the constructor I use my safe RAII unique_ptr-machinery to initialize them. Disadvantages:

  • quite some boiler-plate per item for initialization
  • initialization not done in c'tors init-section
  • members are first ini'd with nullptr and later re-assigned with actual values.

Therefore I am only 70% satisfied with this code.

Is there a more comprehensive way to init Qt5 Widgets in a safe, modern RAII manner in C++14?

\$\endgroup\$
  • \$\begingroup\$ I removed language version tag and placed language tag, so it would be visible on C++ qs list. \$\endgroup\$ – Incomputable Jun 5 '17 at 12:08
  • \$\begingroup\$ @Incomputable right. I should have known that. \$\endgroup\$ – towi Jun 5 '17 at 12:14
  • \$\begingroup\$ Out of curiosity, did you actually encounter any leaks with the old model or is this purely a pre-emptive approach to prevent potential leaks? \$\endgroup\$ – yuri Jun 5 '17 at 12:28
  • 1
    \$\begingroup\$ @yuri Purely pre-emptive for academic reasons. Imagine I design a quite complicated widget on my own that I use all over the place. Maybe that widget throws exceptions (beyond bad_alloc) in its constructor when it fails to initialize (c++-core-guideline-ish). When I have several members of this widget in some big window and the third one throws an exception during its initialization -- I would leak the first two. \$\endgroup\$ – towi Jun 5 '17 at 12:36
3
\$\begingroup\$

When in Qt, act like Qt.

Qt already has an ownership model, and it works by passing the parent object (as a raw pointer) in the constructor (so there's no window of exception-triggered leakage). And Qt has its own weak pointers (including QObjectPointer).

I wouldn't try to outsmart Qt on pointer lifetime and ownership - but keep watching development, and if a future version of the library starts accepting smart pointers to transfer ownership, then reach out and grab it at that time!

In my personal opinion, the code you show is less clear than the Qt-native equivalent:

#include <QWidget>
#include <QVBoxLayout>
#include <QPushButton>
#include <QLabel>
#include <QObject>

class MyWindow : public QWidget {
    Q_OBJECT
public:
    MyWindow(QWidget* parent = nullptr);
private:
    QLabel *const label;
    QPushButton *const button0;
    QPushButton *const button1;
    QVBoxLayout *const layout;
    int count = 0;
private slots:
    void setText();
};

#include <QApplication>
MyWindow::MyWindow(QWidget *parent)
    : QWidget{parent},
      label{new QLabel{"old text", this}},
      button0{new QPushButton{"Increment", this}},
      button1{new QPushButton{"Quit", this}},
      layout{new QVBoxLayout{this}}
{
    layout->addWidget(button0);
    layout->addWidget(button1);
    layout->addWidget(label);
    setLayout(layout);
    connect(button0, &QPushButton::clicked, this, &MyWindow::setText);
    connect(button1, &QPushButton::clicked, qApp, &QCoreApplication::quit);
}
void MyWindow::setText() {
    label->setText(QString{"Label updated %0 times"}.arg(++count));
}

Here, I've made the widget pointers const for safety, so we know that they will be valid for the lifetime of the MyWindow object.

I've also used the method-pointer version of connect() - I recommend this where practicable, to get more checking at compile time. Connect by name only when you really have to, such as when you can't know the complete types of the objects you're connecting. See also Connecting overloaded signals and slots in Qt 5 if you have trouble with overloads.


Your code and comments suggest that you're concerned about leaking memory if an exception is thrown in an initializer. For example, consider a failure to allocate/construct button0 after label was successfully created; will label then be left in limbo? Although our ~MyWindow() destructor is not called (and we don't even need a non-default one), its superclass destructors will be. And that includes the ~QObject destructor, which deletes its children. We know that label will be deleted, because its constructor returned (and adding itself as a child is among the first things it does).

None of this would be true (and we would not be exception-safe) if we forgot to include the optional parent parameter (this) when constructing label; if we were to initialize it without a parent and then its setParent() method in the body of the constructor, we would have a gap where Qt wouldn't own it properly.

We can demonstrate this by wrapping one or more of the allocations in a function. The following code allows you to experiment with throwing exceptions for the different child objects:

int alloc_succeeds = 3;
#include <stdexcept>
template<typename T, typename... Args>
T *createWidget(Args... args)
{
    if (--alloc_succeeds < 0)
        throw std::bad_alloc();
    return new T{args...};
}

#define PARENT this
//#define PARENT nullptr

#include <QApplication>
MyWindow::MyWindow(QWidget *parent)
    : QWidget{parent},
      label{createWidget<QLabel>("old text", PARENT)},
      button0{createWidget<QPushButton>("Increment", PARENT)},
      button1{createWidget<QPushButton>("Quit", PARENT)},
      layout{createWidget<QVBoxLayout>(PARENT)}
{

Change the value of alloc_succeeds to determine how many constructors are successful. If we run the code under Valgrind, we see that by changing PARENT to nullptr, we can get our objects to leak (we also see some of the leaks of Qt itself, so it's helpful to run both to eliminate false positives).

\$\endgroup\$
  • \$\begingroup\$ 1) Yes, I forgot about the c'tor param parent, thx. 2) I am always a bit confused with layouts. They can't be parents because they aren't widgets. Thx for unconfusing me. 3) Problem with raw new in c'tor-init-section: if the 3rd field throws, then the 1st and 2nd pointer are not deleted! This is a no-go for my RAII C++ style. And no magic Qt-mechanism can come to the rescue (that I can see). If there were a rule "QObject constructrors may not throw" (noexcept?) I would be more happy. But this code, I am afraid, I consider unsafe in the general case. Or am I wrong? \$\endgroup\$ – towi Jun 5 '17 at 13:58
  • \$\begingroup\$ Yes, I just had the brainwave that it is safe to raw-new a Widget, if (and only if) it gets a parent as a c'tor argument. \$\endgroup\$ – towi Jun 5 '17 at 15:28
  • \$\begingroup\$ It's a bit surprising if you're not used to Qt; I've updated to show how you can demonstrate what works and what doesn't. As you say, it's safe only if you provide the parent before anything else may throw (passing it as an argument to the constructor is safe and convenient). \$\endgroup\$ – Toby Speight Jun 5 '17 at 15:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.