C++ class to auto delete itself?

SPlatten

I've written a class "clsAlert" this is derived from QDialog. In the class I have a signal which is emitted when the one and only button in the dialog is clicked and in the constructor I connect this signal to a slot in the same class.

    connect(this, SIGNAL(closeClicked()), this, SLOT(cleanup()));

In the cleanup slot:

    void clsAlert::cleanup() {
        delete ui;
        delete this;
    }

In my slot that is attached to the close button:

    void clsAlert::on_btnClose_clicked() {
        close();
        emit closeClicked();
    }

This all seems to work ok, however I have other connections to the same signal so the application is notified when the closeClicked signal is emitted. The problem I'm having is that the signals and slots are performed in the order they were connected, which means that as the first signal connects to the cleanup, no other slots get called.

Is there any way I can fix this, I'm doing this because I need to know the alert dialogs are cleaned up properly.

Thank you

raven-worx

@SPlatten said in C++ class to auto delete itself?:

The problem I'm having is that the signals and slots are performed in the order they were connected, which means that as the first signal connects to the cleanup, no other slots get called.

for such purposes there is the deleteLater() slot

fcarney

@SPlatten said in C++ class to auto delete itself?:

delete this;

This line of code makes me pause. Is it safe to delete an object within itself? I would have never thought of doing that.

JonB

@fcarney
It can be OK if you're careful (e.g. http://www.cs.technion.ac.il/users/yechiel/c++-faq/delete-this.html), but I don't think it's ever safe in a slot....

kshegunov

@fcarney said in C++ class to auto delete itself?:

This line of code makes me pause. Is it safe to delete an object within itself? I would have never thought of doing that.

Yes, it's valid and allowed. Whether it's safe depends on the actual code.

@JonB said in C++ class to auto delete itself?:

I don't think it's ever safe in a slot....

You're correct, it never is. Leads to all kinds of beautiful crashes.

VRonin

delete this;

No. Just no!

I'm doing this because I need to know the alert dialogs are cleaned up properly

Allocate the dialog to the stack and make ui a smart pointer (std::unique_ptr).

As a side note, normally dialogs are closed by calling either accept() or reject() not by deleting itself.

kshegunov

@VRonin said in C++ class to auto delete itself?:

delete this;

No. Just no!

It's fine in general, not for QObjects slots.

@VRonin said in C++ class to auto delete itself?:

Allocate the dialog to the stack and make ui a smart pointer (std::unique_ptr).

Not even gonna touch the useless pointer wrapper here. In fact you should always take the Ui::WhateverClass on the stack. Otherwise you're allocating about 10 pointers or so, so when you call setupUi, you can allocate 10 times by one pointer and only then they are going to allocate the 10 objects. That's just stupid, sorry. What you can even do is not to keep the ui object at all, if you'd designed the widget well and have tied everything in the constructor with signals and slots.

VRonin

@kshegunov said in C++ class to auto delete itself?:

In fact you should always take the Ui::WhateverClass on the stack

What's the difference between a stack allocated Ui::WhateverClass and a std::unique_ptr<Ui::WhateverClass>? I mean in real performance terms it's just a bunch of pointers, right? (I don't use designer so not that familiar with this)

so when you call setupUi, you can allocate 10 times by one pointer and only then they are going to allocate the 10 objects

I'm not sure I follow

kshegunov

Let me illustrate with an excerpt:


class Ui_LoginDialog
{
public:
    QVBoxLayout *verticalLayout;
    QLabel *titleLabel;
    QGroupBox *credentialsGroupBox;

// ...
    void setupUi(QDialog *LoginDialog)
    {
        if (LoginDialog->objectName().isEmpty())
            LoginDialog->setObjectName(QStringLiteral("LoginDialog"));
        LoginDialog->resize(450, 224);
        verticalLayout = new QVBoxLayout(LoginDialog);
        titleLabel = new QLabel(LoginDialog);
        credentialsGroupBox = new QGroupBox(LoginDialog);
// ...

setupUi already does exactly what you'd do manually. You don't need to allocate the pointers to the objects on the heap too; at least that's one new you can spare.

I'm not sure I follow

Referring to the public -> private object allocations.
Basically you new a bunch of pointers. Then for each one you allocate one pointer, and after that you allocate the actual data (i.e. the private object).