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!

Silsby

You should read the Qt docs and understand how Qt manages dynamically allocated objects. – Jacy

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.

Insurer

Ok, I understand. But what if I just need a method that simply deletes the buttons created in the constructor? Suppose in the future I will not destroy the entire object of ButtonWidget type. And suppose I just need to delete buttons and maybe another method to add new buttons? – Fenestration

Simply delete the parent, or delete the buttons. I stand corrected, but it should work fine. – Insurer

You will have to do the management yourself. With SomeQObject->setParent(0) you can detach an object from its parent. Then you can simply delete it. – Fleecy

@WernerErasmus: Would something like this do the job: QLayoutItem *child; while ((child = gridLayout->takeAt(0)) != 0) { delete child->widget(); delete child; } ? – Fenestration

Just delete the parent, and it's children will be deleted automatically... If you delete a child, make sure to set it's parent to null, as mentioned by @Fleecy – Insurer

@WernerErasmus: Simplifying things: Suppose the class is initialised with a list of 6 buttons labeled as "Text_1"... "Text_6". Now suppose I need to delete only the last 2 buttons (i.e. "Text_5 and Text_6). What is the correct way to do that? – Fenestration

I've adapted my answer to answer your question. Just delete the buttons. No setting of parents to null etc as mentioned by @AndeasT if the scope of the parent exceeds that of the children. – Insurer

@groenhen. See my above comment and adapter answer, as well as link provided. – Insurer

Not quite... Only asked what would happen if he wanted to delete a sub-object. (no parent destruction whatsoever). I found this a minute ago: #17704013 – Fenestration

@groenhen, and the link I've provided indicates exactly that (deleting sub-objects...read). It will simply be removed from the parents list. The answer you've sighted (where the OP answers himself) works, but is an absolute overkill. The answer below that is more correct - just delete it....! – Insurer

My major concern is still about memory leaks, as I intend to use that example in my project: deleting and adding buttons in an app that will run 24/7... So, that is why I asked for the safe and correct way of doing it... I didn't find a firm answer yet... but voted up all :) – Fenestration

@groenhen. Why don't you test it (if you don't believe the documentation and can't read the sourcecode...). Inherit from QButton, write a destructor that prints out and check if it is deleted. Simple as that. You most certainly won't have a leak. – Insurer

Well, I tested today: step1) I initialized the ButtonWidget with 6 texts/elements. All displayed ok: 3 on first row, 3 on 2nd row. step2) I deleted all 6 buttons/elements and then I tried to add 10 new items... To my big surprise, the new items were added but in a strange manner: they were stretched in the space that was previously occupied by the initial 6 elements... ?!? – Fenestration

Seems like you did not add them to the layout correctly. This is totally independent of whether the previous buttons were deleted correctly, but hey: Drop us all the sources and some money and we might fix it ;-) – Insurer

@WernerErasmus: Complete source-code is here, as it turned out to be a slightly different topic now : #36773442 . Best regards, SG. – Fenestration

@groenhen. Thanks. I'll look at it some time and try to find the best solution. – Insurer

@WernerErasmus: Thanks in advance. I'll check-back on SO late at night... today is my birthday and I'll try to spend more time with family, ... That's life - we're getting older and... wiser! hehe :) – Fenestration

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).

Pagandom

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.

Fleecy

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.

Mess