Qt: What is the correct and safe way to write the destructor of this class?

Question

I am using Qt5 on Windows7 and I recently found an interesting Qt example code.

Basically, it looks like this:

ButtonWidget::ButtonWidget(const QStringList &texts, QWidget * parent) 
: QWidget(parent)
{
    signalMapper = new QSignalMapper(this);

    QGridLayout * gridLayout = new QGridLayout;
    for (int i = 0; i < texts.size(); ++i) 
    {
        QPushButton * button = new QPushButton(texts[i]);
        connect(button, SIGNAL(clicked()), signalMapper, SLOT(map()));
        signalMapper->setMapping(button, texts[i]);
        gridLayout->addWidget(button, i / 3, i % 3);
    }

    connect(signalMapper, SIGNAL(mapped(QString)), this, SIGNAL(clicked(QString)));

    setLayout(gridLayout);
}

It is a nice and useful example, but it doesn't have a proper destructor... just in case I want to delete an object of ButtonWidget type, or if I want to customise the code to be able to remove/add widgets. The idea is how to delete all objects created in the constructor (dynamically, using new).

My approach was to use a private variable QList<QPushButton*> list, add all new-allocated buttons to list (in the constructor) and delete them one-by-one in the destructor, using the above list. But it seems so kindergarten-approach.

I think there must be some other way, better way to do it, without a list and without messing the constructor code :) Thanks for your time and patience!

Answer 1

With Qt classes that receive parent as arguments to their constructors, e.g QSignalMapper, it is important to note that the class adds itself to its parent's object list, and it will be destructed when its parent (a QObject) is destructed.

Therefore, if you pass an object a parent, you don't have to do anything. You might have an empty destructor in your cpp file, just to make sure the definition of the member is available to QObject, but that depends on the class.

However, if you do choose to write your own destructor, a typical Qt "child" that is implemented correctly will remove itself from its parent at destruction.

In addition to the question posed in the caption/title, The OP asked what would happen if he wanted to delete a sub-object prior to deleting the parent:

Sighting the following, it seems (and this is iaw. my own experience) that he may just delete the child widgets, and that they will remove themselves. It has been mentioned that one can set the parent of a child to null, but this is not necessary, except if one would want to delete the parent and keep the child alive (re-parenting...).

However, as shown in the last paragraph of the example, the order of instantiation is important. If a parent is instantiated after a child, and the child's parent is set explicitly, a dead reference / invalid pointer to the parent will be left with the child, and undefined behaviour will happen at destruction of the child when it tries to remove itself from the out-of-scope parent.

In addition to Qt's documentation, one can also look at the implementation of QObject::setParent here (setParent_helper). From this it can be seen that they go to great effort to allow for children/parents deletion to happen without mishaps, except for the mentioned case.

Answer 2

From QWidget::setLayout:

The QWidget will take ownership of layout.

From QLayout::addItem (called by QLayout::addWidget):

Note: The ownership of item is transferred to the layout, and it's the layout's responsibility to delete it.

---

You don't have to clean up anything. Manage the widgets through the layout
(addWidget, removeWidget/removeItem).

Answer 3

You have to see to it, that you hook all your widgets into Qt's object tree, which will then take care of destruction for you. You can do that by giving it a parent when constructing it. Like you did with the SignalMapper.

Answer 4

It's not true that there's no "proper" destructor. There is a destructor - one generated by the compiler - and that destructor does everything necessary to free the resources. That's how it should be, and that's how modern C++ code should be designed.

Properly designed C++ classes should be usable without having to explicitly manage their resources. That is the case here.

Furthermore, the example unnecessarily dynamically allocates members that could have been simply class members. In C++11 there's also no need for the signal mapper. This is how I'd do it:

class ButtonWidget : public QWidget {
  Q_OBJECT
  QGridLayout m_layout { this };
public:
  ButtonWidget(const QStringList &items, QWidget *parent = 0);
  ~ButtonWidget();
  Q_SIGNAL void buttonClicked(const QString &);
}

ButtonWidget::ButtonWidget(const QStringList &items, QWidget *parent) 
: QWidget(parent)
{
  const int columns = 3;
  for (int i = 0; i < items.size(); ++i) {
    auto text = items[i];
    auto button = new QPushButton(text);
    connect(button, &QPushButton::clicked, [this, text]{
      emit buttonClicked(text);
    });
    m_layout.addWidget(button, i / columns, i % columns);
  }
}

ButtonWidget::~ButtonWidget() {}

This is a complete, usable widget, with no memory leaks. That's how modern C++/Qt should look. If you need to do something fancy in the destructor, you should always consider factoring out the memory management into a RAII class of its own. For example, instead of manually closing a file handle in the destructor, consider the use of QFile, or writing a similar resource-managing class that you can then use without worrying about manually managing the lifetime of the handle.