[SOLVED] Exception safety in widget constructors?

Sydius

"This tutorial":http://doc.qt.nokia.com/4.7/gettingstartedqt.html has me create a constructor that looks like this:

@
Notepad::Notepad()
{
textEdit = new QTextEdit;
quitButton = new QPushButton(tr("Quit"));

         connect(quitButton, SIGNAL(clicked()), this, SLOT(quit()));

         QVBoxLayout *layout = new QVBoxLayout;
         layout->addWidget(textEdit);
         layout->addWidget(quitButton);

         setLayout(layout);

         setWindowTitle(tr("Notepad"));
     }@

Presumably the layout takes ownership of the widgets and the widget takes ownership of the layout (at least that is what I determined after trying to fix a double-delete from the code in the first example on that page). However, it seems to be the case that, should the allocation of QVBoxLayout throw an exception (as an example), textEdit and quitButton would never be cleaned up.

How is this commonly avoided in Qt code, where ownership of the pointers is taken during construction? I would normally just use a smart pointer for the involved allocations if ownership wasn't to be taken immediately.

giesbert

Hi,

as it seams that textEdit and PushButton are children of Notepad, why don't you give them parent pointers?:

@
Notepad::Notepad()
{
textEdit = new QTextEdit(this);
quitButton = new QPushButton(tr("Quit"), this);

connect(quitButton, SIGNAL(clicked()), this, SLOT(quit()));

QVBoxLayout *layout = new QVBoxLayout;
layout->addWidget(textEdit);
layout->addWidget(quitButton);

setLayout(layout);

setWindowTitle(tr("Notepad"));

}
@

I didn'T try this, but perhaps it solves your problem?

By the way, if the allocation of a layout does not work anymore, you really have a problem. It there is no more memory usable, the application will not work anymore. so in my opinion, the only solution would be close the application (because all events, signals etc. also create stuff on the heap). If you close the app, all memory of the application will be freed (definitly under windows, but I think also on Mac or Linux).

What else do you want to do, if some objects cant be created?

Sydius

Thank you for your help.

[quote author="Gerolf" date="1294128937"]as it seams that textEdit and PushButton are children of Notepad, why don't you give them parent pointers?[/quote]

Does doing so cause ownership to remain with Notepad? If so, I would be able to assign them to smart pointers, which should alleviate my concerns.

[quote author="Gerolf" date="1294128937"]By the way, if the allocation of a layout does not work anymore, you really have a problem. It there is no more memory usable, the application will not work anymore. so in my opinion, the only solution would be close the application (because all events, signals etc. also create stuff on the heap). If you close the app, all memory of the application will be freed (definitly under windows, but I think also on Mac or Linux).

What else do you want to do, if some objects cant be created?[/quote]

While this is a contrived example in which it is probably only possible to simply terminate the application, my question was meant as a more general one regarding the typical solution to this problem as there are many reasons an exception might be thrown from a constructor, and I would like to guard against these as intelligently as is reasonable. When I read that example, it stuck out in my mind as one of those red flags I've been trained to notice and would assume has been dealt with in some common way by most Qt developers, but wasn't immediately obvious to me.

giesbert

bq. Does doing so cause ownership to remain with Notepad? If so, I would be able to assign them to smart pointers, which should alleviate my concerns.

This gives parent - child behavior that means, the base destructor should destroy it.

If I have some stuff to do in the constructor which might throw exceptions, I normally do try/catch and handle that bxy myself. so the constructor goes well but some part of the object might then not work or give special result codes. If it is needed to the client, it could also be done with constructor in combination with an init function (not nice but works :-) ).

Sydius

[quote author="Gerolf" date="1294140022"]This gives parent - child behavior that means, the base destructor should destroy it.[/quote]

Is that to say that the Notepad destructor should destroy it (IE I have to write it) or that the QWidget base class destructor will? If it's the latter, I wouldn't be able to store them in smart pointers in the usual way and the destructors would never be called since the Notepad object would not have been fully constructed yet.

[quote author="Gerolf" date="1294140022"]If I have some stuff to do in the constructor which might throw exceptions, I normally do try/catch and handle that bxy myself. so the constructor goes well but some part of the object might then not work or give special result codes. If it is needed to the client, it could also be done with constructor in combination with an init function (not nice but works :-) ).[/quote]

Using a try/catch block may be the best option I can think of in this case, though it seems far less than ideal. I don't think I would need to create/check for zombie classes since, assuming I caught the exceptions, I could just do the cleanup in the constructor itself and rethrow them. I'm still hoping for a more elegant solution, if one exists.

giesbert

The base class destructor should do it, as it shall be called as the base class constructor was comleted, won't it? exceptions in a constructor are always a bit strange in how to handle them....

Sydius

[quote author="Gerolf" date="1294164371"]The base class destructor should do it, as it shall be called as the base class constructor was comleted, won't it? exceptions in a constructor are always a bit strange in how to handle them....
[/quote]

Not if an exception is thrown from the constructor of Notepad, as Notepad would not have been fully constructed and the destructor would not know what portion to undo. Assuming I caught the exception in the constructor and did not rethrow it, then yes, you are correct, as the constructor would then complete. I would, however, be forced to check to see if Notepad were a "zombie" class as a second step of creating an instance of Notepad and that seems messy at best. (see "this C++ FAQ":http://www.parashift.com/c++-faq-lite/exceptions.html#faq-17.8 for more information)

goetz

Sydius, that's right. The base class destructor can't clean up the mess the was made in the derived class' constructor. It has to be cleaned up there. I'm not sure if the base class destructor is even called in that case. IMHO, the best way is to wrap the critical code in a try block and to clean up in the catch block if something goes wrong. The pointers should be set to null before starting the try:

@
Notepad::Notepad()
: textEdit(0),
quitButton(0)
{
QVBoxLayout *layout = 0;

try {
    textEdit = new QTextEdit(this);

quitButton = new QPushButton(tr("Quit"), this);

     connect(quitButton, SIGNAL(clicked()), this, SLOT(quit()));

    layout = new QVBoxLayout;
     layout->addWidget(textEdit);
    layout->addWidget(quitButton);

     setLayout(layout);
} catch(MyFancyException &e) {
if(textEdit)
delete textEdit;

    if(quitButton)
        delete quitButton;

    if(layout)
        delete layout;

    throw e;
}

setWindowTitle(tr("Notepad"));
}
@

ixSci

There is no need in smart pointers or try blocks here. In Qt the QObject class is responsible for cleaning up during its destruction. Also QObject is a parent class of the majority of Qt classes include the GUI ones. Although your Notepad object has not been created yet its QObject parent was. Thus you'ill have no leaks if you properly set parent at the construction time.

Sydius

[quote author="ixSci" date="1294168106"]There is no need in smart pointers or try blocks here. In Qt the QObject class is responsible for cleaning up during its destruction. Also QObject is a parent class of the majority of Qt classes include the GUI ones. Although your Notepad object has not been created yet its QObject parent was. Thus you'ill have no leaks if you properly set parent at the construction time. [/quote]

Ahh, actually you're correct. I was mistaken in believing that the base class destructor would not be called. Thank you for this clarification, it seems that Gerolf's original solution will be sufficient!

Panke

It is sufficient except that the layout (and the widgets added to it) will leak before the call to setLayout()

ixSci

Qt claims its classes are exceptions free(bold statement but almost true) that is why there is no leaks at the code presented above.

goetz

The layout does not have a parent and is therefore not caught by the QObject destructor, that's right. The layout is in the parent-child hierarchy only after setLayout() in line 17.

ixSci

Volker, exactly. But there are no possible exceptions between layout creation and its adoption by the QObject.
This ctor seems truly exception neutral to me.

goetz

In this very case, yes. But if someone does some other object creation or manipulation inbetween that can throw an exception, it can raise a problem.

Sydius

[quote author="Volker" date="1294174797"]In this very case, yes. But if someone does some other object creation or manipulation inbetween that can throw an exception, it can raise a problem.[/quote]

Having just "looked it up":http://doc.trolltech.com/latest/qvboxlayout.html, it appears that QVBoxLayout can take a parent, which should alleviate this concern?

BlackDante

[quote author="Volker" date="1294166366"]
@
Notepad::Notepad()
: textEdit(0),
quitButton(0)
{
QVBoxLayout *layout = 0;

try {
    textEdit = new QTextEdit(this);

quitButton = new QPushButton(tr("Quit"), this);

     connect(quitButton, SIGNAL(clicked()), this, SLOT(quit()));

    layout = new QVBoxLayout;
     layout->addWidget(textEdit);
    layout->addWidget(quitButton);

     setLayout(layout);
} catch(MyFancyException &e) {
if(textEdit)
delete textEdit;

    if(quitButton)
        delete quitButton;

    if(layout)
        delete layout;

    throw e;
}

setWindowTitle(tr("Notepad"));
}
@[/quote]

Hi, I have a question no related with topic but about this code. Why function "throw" is in a catch block? It is possible to do that? In this example I am little confused because I always been learnig that try block must throw the exception and in catch we catching this exception, in my university it is somethink like hard rule.

Sydius

[quote author="BlackDante" date="1294179880"]
Hi, I have a question no related with topic but about this code. Why function "throw" is in a catch block? It is possible to do that? In this example I am little confused because I always been learnig that try block must throw the exception and in catch we catching this exception, in my university it is somethink like hard rule. [/quote]

It is common practice to "rethrow" an exception from a catch block after handling it if it is not the top-level catch. This allows it to propagate to the top-most catch block, if appropriate. Having more than a top-level try/catch block is to be avoided as it's typically unnecessary (as is true in this case), and it makes little sense to rethrow from the top-most catch, which might be where you got this impression. When they do have to be nested, though, it's usually appropriate to rethrow it.

BlackDante

oh, now I understand :) thanks for explain :)

ixSci

Also you don't have to specify an object when you need to rethrow exception inside a catch block. You just need to type throw; and it will rethrow source object. Moreover you can use some languages tricks such as automated rethrow when using try\catch function block in a ctor.
Example:
@Notepad()
try
{
//blalala
throw "Exception";
}catch(...)
{
//it will rethrow automatically
}@