2

I am following Qt examples (like TabDialog) and I notice that all the UI items are created as pointers - yet I see no delete and no destructor.

Is that right ? Will that not lead to memory leak ?

I am trying to add destructors

~TabDialog()
{
    delete tabWidget;
    delete buttonBox;
}

and on caller

TabDialog *tabDialog = new TabDialog();
tabDialog->setAttribute(Qt::WA_DeleteOnClose);
tabDialog->exec();

But the program crashes when I close the dialog.

Are the destructors, and delete all pointer items, unnecessary or am I doing it wrong ?

Kuba hasn't forgotten Monica
  • 88,505
  • 13
  • 129
  • 275
Thalia
  • 11,305
  • 15
  • 71
  • 169
  • If you construct dialog's widgets and set the dialog as a parent, you don't need to delete them in the dialog's destructor: Qt will take care about their deletion. – vahancho Jun 30 '15 at 13:58
  • Sorry, this doesn't reproduce. Your code is correct. The deletion is unnecessary, but harmless. – Kuba hasn't forgotten Monica Jun 30 '15 at 17:37
  • @KubaOber Thank you, I was not sure because I expect a pattern of new + delete and I did not want to make memory leaks – Thalia Jun 30 '15 at 17:43
  • Are you sure that the code immediately following `tabDialog->exec()` is not trying to access `tabDialog`? Remember that when `exec()` returns, `tabDialog` is a dangling pointer. Perhaps you have `delete tabDialog` right below it, and you're hiding it from us?! – Kuba hasn't forgotten Monica Jun 30 '15 at 17:51
  • There is no code following the `tabDialog->exec()`... there is no other code on the function stated, which is called as `menu.addAction(showPropertiesAct);` in a context menu event. – Thalia Jun 30 '15 at 18:12
  • You really need to post your code. The Qt example you refer to has no line containing `menu.addAction`. So you don't have the excuse that you are simply "using" Qt example code. You are using code that has been modified, and since you don't understand the impact of your changes, you must post it all, after minimizing away all the parts that don't impact the problem you're seeing. This is debugging 101, and we can't help you if you don't do this. **All I can tell you is that if you merely add the changes you show above to the Qt example, it still works.** – Kuba hasn't forgotten Monica Jun 30 '15 at 18:21
  • -1 since the question is incomplete without code that actually fails. Everything you show above, even the menu code, is OK if you simply add it to the Qt's example code. This is why questions like this are so bad: they are an extreme time drain - I entirely too often assume that someone new might use Qt in a way that the designers haven't thought of and finds a genuine bug or at least an undocumented corner case. This almost never turns out to be, and instead it's a wild goose chase because the asker doesn't seem bothered to follow the clearly spelled out rules. – Kuba hasn't forgotten Monica Jun 30 '15 at 18:34
  • Here's a question asking hint: Forget about any code you wrote, and then look at the question from the perspective of someone who doesn't know anything about what you're doing. If you can't do it while looking at the *question only*, and perhaps some relatively invariant publicly available code (say Qt examples that you *should link to*), then your question isn't complete. – Kuba hasn't forgotten Monica Jun 30 '15 at 18:36

3 Answers3

4

I think that you are confused because of these lines:

tabWidget = new QTabWidget;//and so on

You don't see explicit parent (like new QTabWidget(this);), but it is not necessary here. Take a look here:

QVBoxLayout *mainLayout = new QVBoxLayout;
mainLayout->addWidget(tabWidget);
mainLayout->addWidget(buttonBox);
setLayout(mainLayout);

setLayout will reparent your QVBoxLayout and QVBoxLayout will reparent all widgets inside it, so now your widgets has a parent and they will be destroyed after your dialog.

As doc said:

When you use a layout, you do not need to pass a parent when constructing the child widgets. The layout will automatically reparent the widgets (using QWidget::setParent()) so that they are children of the widget on which the layout is installed.

Note: Widgets in a layout are children of the widget on which the layout is installed, not of the layout itself. Widgets can only have other widgets as parent, not layouts.

Kosovan
  • 16,817
  • 2
  • 40
  • 44
  • is `tabDialog->setAttribute(Qt::WA_DeleteOnClose);` a good idea to add ? or is that default ? – Thalia Jun 30 '15 at 14:34
  • @Thalia It will delete dialog when user will close it. It is very good if you need this and if you call dialog with `tabDialog->show();` because in your case `exec()` "stops" execution and wait result, so you can write something like `tabDialog->exec(); delete tabDialog;` – Kosovan Jun 30 '15 at 14:38
  • Your answer is misleading. It is not invalid to destruct widgets that are under Qt memory control. The problem here is that `delete` is passed dangling pointers. Thalia is not deleting widgets that exist - that would be a perfectly valid thing to do. "Qt" will not delete widgets that you've already deleted, that'd be very bad. Qt in fact tracks lifetimes of `QObject`s with parents, and acts very appropriately if you delete the objects first. **Again, the error here is that `delete` is passed dangling pointers that don't point to valid `QObject` instances but to junk!** – Kuba hasn't forgotten Monica Jun 30 '15 at 17:17
  • If you were right, this code wouldn't work: `int main(int argc, char ** argv) { QApplication app(argc, argv); QWidget w; QVBoxLayout l(&w); QLabel label("Hello"); QPushButton button("Button"); l.addWidget(&label); l.addWidget(&button); w.show(); return app.exec(); }` Yet it works in spite of the following destructor calls upon exit from `main`, in order: 1. `button.~QPushButton()`, 2. `label.~QLabel()`, 3. `l.~QVBoxLayout()`, 4. `w.~QWidget()`, 5. `app.~QApplication()`. These calls are valid C++, BTW. – Kuba hasn't forgotten Monica Jun 30 '15 at 17:21
  • @Chernobyl In C++, objects are destructed in order from most derived to base class. `TabDialog::~TabDialog()` will execute way before `TabDialog::~QObject()`. – Kuba hasn't forgotten Monica Jun 30 '15 at 17:30
  • @KubaOber Sorry, my last comment was a really stupid... I just can't understand, why in this case pointers are dangling, you think that there is something wrong in another code (which we don't see)? – Kosovan Jun 30 '15 at 17:41
  • @Chernobyl Yes, something is wrong in some other code. I really didn't want to dig into that example. – Kuba hasn't forgotten Monica Jun 30 '15 at 17:45
  • @KubaOber I removed wrong line with delete and currently answer contains correct information about reparenting in layout etc. It is the maximum what can I do. – Kosovan Jun 30 '15 at 18:52
  • "your widgets has a parent and they will be destroyed before your dialog" - this is still misleading. The child widgets will be destroyed by `QObject::~QObject`, or perhaps by `QWidget::~QWidget`. Per C++ semantics, these will run *after* the dialog's class-specific destruction has been done - after any user-provided code in `TabDialog::~TabDialog`. The widgets will be destructed sometime after `TabDialog::~TabDialog` user code has ran, if it hasn't destructed them itself of course, and before `QObject::~QObject` finishes, as it is the most base destructor available. – Kuba hasn't forgotten Monica Jun 30 '15 at 19:02
  • @KubaOber Done. I know ctor/dtor calls order (base-derived;derived-base), because it is very basic, but compiler in my head let me down today, just not my day. Anyways, thank you for pointing and for your time(it was really long) – Kosovan Jun 30 '15 at 19:20
3

I'm sorry, but this just doesn't reproduce. The test case is below. You'd probably need to add some extra code to make it reproduce.

Qt's memory management will take care of everything, since all of the widgets end up having parents. Namely:

  1. Tabs are parented as soon as they are passed to addTab.
  2. tabWidget and buttonBox are parented as soon as they are added to the layout.

Since you delete the tabWidget and buttonBox before Qt attempts to delete them, everything is fine. As soon as you delete them, QObject's memory management is informed and they are removed from the TabDialog's child list. I've made this point explicit in the destructor code.

The meaning of Q_ASSERT is: "At this point during runtime, the following must be true". If we're wrong, the debug build will abort. Since it doesn't, the assertions are correct. Thus, before delete tabWidget, the dialog has both QTabWidget and QDialogButtonBox children. After delete tabWidget, the dialog should not have any QTabWidget children anymore. And so on.

#include <QApplication>
#include <QDialog>
#include <QTabWidget>
#include <QDialogButtonBox>
#include <QVBoxLayout>

class TabDialog : public QDialog
{
   QTabWidget *tabWidget;
   QDialogButtonBox *buttonBox;
public:
   TabDialog() :
      tabWidget(new QTabWidget),
      buttonBox(new QDialogButtonBox(QDialogButtonBox::Ok | 
                                     QDialogButtonBox::Cancel))
   {
      tabWidget->addTab(new QWidget, tr("General"));
      tabWidget->addTab(new QWidget, tr("Permissions"));
      tabWidget->addTab(new QWidget, tr("Applications"));
      QVBoxLayout *layout = new QVBoxLayout;
      layout->addWidget(tabWidget);
      layout->addWidget(buttonBox);
      setLayout(layout);
      connect(buttonBox, SIGNAL(accepted()), this, SLOT(accept()));
      connect(buttonBox, SIGNAL(rejected()), this, SLOT(reject()));
   }
   ~TabDialog() {
      Q_ASSERT(findChild<QTabWidget*>());
      Q_ASSERT(findChild<QDialogButtonBox*>());
      delete tabWidget;
      Q_ASSERT(! findChild<QTabWidget*>());
      Q_ASSERT(findChild<QDialogButtonBox*>());
      delete buttonBox;
      Q_ASSERT(! findChild<QTabWidget*>());
      Q_ASSERT(! findChild<QDialogButtonBox*>());
   }
};

int main(int argc, char *argv[])
{
   QApplication a(argc, argv);
   TabDialog *tabDialog = new TabDialog();
   tabDialog->setAttribute(Qt::WA_DeleteOnClose);
   tabDialog->exec();
   return 0;
}

The only way it'd crash is if you tried the following:

int main(int argc, char *argv[])
{
   QApplication a(argc, argv);
   TabDialog *tabDialog = new TabDialog();
   tabDialog->setAttribute(Qt::WA_DeleteOnClose);
   tabDialog->exec();
   // At this point `tabDialog` is a dangling pointer.
   delete tabDialog; // crash
   return 0;
}

Unfortunately, Qt examples are a case of pointless premature pessimization. Qt's classes utilize the PIMPL idiom extensively. Thus the size of, say a QTabWidget is not much larger than that of QObject (48 vs. 16 bytes on my 64 bit platform). By allocating the fixed members of your class on the heap, you're performing two heap allocations: a small one for the QObject-derived class, and then another one for its PIMPL. You're doubling the number of allocations for no good reason.

Here's how to avoid this pessimization:

#include <QApplication>
#include <QDialog>
#include <QTabWidget>
#include <QDialogButtonBox>
#include <QVBoxLayout>

class TabDialog : public QDialog
{
   QVBoxLayout m_layout;
   QTabWidget m_tabWidget;
   QDialogButtonBox m_buttonBox;
   QWidget m_generalTab, m_permissionsTab, m_applicationsTab;
public:
   TabDialog() :
      m_layout(this),
      m_buttonBox(QDialogButtonBox::Ok | QDialogButtonBox::Cancel)
   {
      m_tabWidget.addTab(&m_generalTab, tr("General"));
      m_tabWidget.addTab(&m_permissionsTab, tr("Permissions"));
      m_tabWidget.addTab(&m_applicationsTab, tr("Applications"));
      m_layout.addWidget(&m_tabWidget);
      m_layout.addWidget(&m_buttonBox);
      connect(&m_buttonBox, &QDialogButtonBox::accepted, this, &QDialog::accept);
      connect(&m_buttonBox, &QDialogButtonBox::rejected, this, &QDialog::reject);
   }
};

int main(int argc, char *argv[])
{
   QApplication app(argc, argv);
   auto tabDialog = new TabDialog();
   tabDialog->setAttribute(Qt::WA_DeleteOnClose);
   tabDialog->show(); // NOT tabDialog->exec()!!
   return app.exec();
}

The less explicit heap allocations, the better. This way you can't even be tempted to delete anything in the destructor, since there are no pointers involved. The compiler generates necessary destructor calls for you automatically.

Furthermore, if you're showing only one window in main, there's no point to explicit heap allocation either:

int main(int argc, char *argv[])
{
   QApplication app(argc, argv);
   TabDialog tabDialog;
   tabDialog.show();
   return app.exec();
}
Community
  • 1
  • 1
Kuba hasn't forgotten Monica
  • 88,505
  • 13
  • 129
  • 275
  • Not sure about being able to delete child objects - My program froze when I tried to delete the child items. But I understand now why I don't need to delete them. – Thalia Jun 30 '15 at 17:46
  • @Thalia Per SO's rules, if you have questions about particular code, it must be posted in the question itself. You need to take Qt's example and start minimizing it to figure out why it fails. It's your job. Your question, as it stands, is off-topic because of this. Or you can take the minimized example from my answer, and start adding code from Qt's example to it, and see at what point will it fail. My hunch is that you won't be able to reproduce the problem. Try wiping your build directory and do a fresh build. My bet is that it will work fine. – Kuba hasn't forgotten Monica Jun 30 '15 at 17:49
1

Memory handling on Qt

Qt handles the widgets as a tree, every widget has a parent and every parent has the obligation to free the children memory, if a widget has no parent you should delete it manually with the operator delete.

  • This is misleading. The error here is that `delete` is passed dangling pointers: pointers to objects that don't exist anymore. It's perfectly valid (and common) to destruct or delete `QObject`s that have parents! – Kuba hasn't forgotten Monica Jun 30 '15 at 17:24