4

So, I've learned so far, that Qt releases the memory of all child objects when a parent object gets deleted. Also, one generally doesn't have to care about memory management for objects created on the stack (i.e. NOT as pointers).

Now, when I did the very good "AddressBook" tutorial, I found this in part 5:

AddressBook::AddressBook(QWidget *parent) : QWidget(parent)
{
    dialog = new FindDialog;
}

Complete source is available: addressbook.h addressbook.cpp finddialog.h

Here, dialog is a private member of AddressBook, and it is a pointer to a FindDialog. FindDialog inherits QDialog, but no this-Pointer is passed to the constructor (as seen above). No explicit destructor exists, there is no delete dialog-call...

Also, not passing this seems to be intentional:

[The FindDialog's] constructor is defined to accept a parent QWidget, even though the dialog will be opened as a separate window.

Wouldn't this cause a memory leak? Or is there some other mechanism that will silently delete dialog and free its memory?

Thanks in advance for any help!

Update: I posted this issue to the qt-project.org forums and it should get fixed soon.

Alex K
  • 90
  • 7
  • 1
    Nice. That is almost certainly a memory leak, unless there is some real magic happening with the `Q_OBJECT` macro. You can verify this with certainty using [valgrind](http://valgrind.org/). However, it's irrelevant, since `AddressBook` lasts the lifetime of the application. Still, in a tutorial, it should have been freed, precisely to avoid raising concerns such as yours. – Matt Phillips Apr 21 '14 at 10:43
  • @MattPhillips But `AddressBook` only lasting the lifetime of the application doesn't influence `dialog`, does it? It would, if it had been created on the stack, but not in this case here... – Alex K Apr 21 '14 at 11:32
  • 1
    The point is, the `AddressBook` instance is only destroyed when the application terminates, at which point all memory associated with it, including `dialog`, is freed. So a memory leak is avoided, but in a stylistically poor and unscalable way. – Matt Phillips Apr 21 '14 at 11:34
  • As far as I'm concerned, the member should be declared as follows: `FindDialog dialog`. Then you don't need to explicitly allocate nor construct it, and it cannot leak. There's no good reason to use a pointer here at all. – Kuba hasn't forgotten Monica Apr 21 '14 at 15:17
  • It's also possible that the `FindDialog` frees itself when one close the (modal?) dialog window... – Massa Apr 21 '14 at 15:18
  • 1
    @Massa: That's certainly possible, but there perhaps should be a comment to that effect. The use of a pointer is still an unecessary complication. – Kuba hasn't forgotten Monica Apr 21 '14 at 15:18
  • Agreed! And even if the dialog frees itself, the pointer will dangle... – Massa Apr 21 '14 at 15:20
  • @Massa: not with QPointer, no. – László Papp Apr 22 '14 at 06:25

1 Answers1

3

There is no excuse for this, and it eventually has more issues than you just bring up, namely:

  • It is not managed as you say.

  • It does not use the conventional new Foo() syntax.

  • It is not done in the constructor's initializer list.

The OS will probably free this up once the application quits, but still, I always speak up against such issues, anyhow, especially in example projects. The appropriate fix would be to use either a stack object instead of the heap object or QPointer in my opinion.

See the following post for details in case the latter:

How to crash (almost) every Qt/KDE Application and how to fix

This should be reported and fixed upstream; good catch!

I have just submitted a change to Gerrit about this in here.

László Papp
  • 51,870
  • 39
  • 111
  • 135
  • Thanks for the info and the link to the blog-entry.. very interesting! I just posted on the qt-project.org-Forum suggesting to fix this. – Alex K Apr 21 '14 at 12:36
  • Additional note about `new Foo` vs `new Foo()` syntax bullet point: since `Dialog` is a *non-POD type*, they do exactly same thing for it, but using `()` would still be good practice. More about it for example [here](http://stackoverflow.com/a/620402/1717300). – hyde Apr 22 '14 at 06:31
  • @hyde: Even non-POD types fall into the [non-initialized vs. default/value initialized difference](http://stackoverflow.com/a/620402/2682142) with C++03 and on. They were similarly uninitialized in C++98, so it will also depend on the standard variant. But as the update writes, there is a change submitted now to Gerrit which makes the object stack rather than heap. – László Papp Apr 22 '14 at 06:44