Should I delete QPointer in the destructor of the class?

Question

Suppose I have a class like the following:

#include <Object>
#include <QProcess>

class MyClass: public QObject {
private:
    QPointer<QProcess> m_process{ nullptr };
public:
    MyClass(QObject *parent = nullptr)
        : QObject{ parent }
        , m_process{new QProcess{} }
    {
        QObject::connect(m_process, &QProcess::errorOccurred,
            this, [](QProcess::ProcessError error) {
            qDebug() << "error: " << error;
            });
    }
    ~MyClass()
    {
        if (m_process)  delete m_process; //--> is it necessary?
    }
};

Should I need to have the delete the m_process manually as shown in the destructor?

Unfortunately, I can not use std::unique_ptr or std::shared_ptr, as of

QObject::connect(m_process, &QProcess::errorOccurred,
            this, [](QProcess::ProcessError error) {
            qDebug() << "error: " << error;
            });

I have not seen a proper overload for QObject::connect.

On the other hand in QPointer::~QPointer() I have read:

Destroys the guarded pointer. Just like a normal pointer, destroying a guarded pointer does not destroy the object being pointed to.

That means QPointer::~QPointer() will delete as MyClass's object go out of scope, and hence I am deleting the m_process twice?

Or did I misunderstood?

You don't have to. You can use QSharedPointer or QScopedPointer as an alternative. — mohabouje, May 04 '19 at 17:19
"I have not seen a proper overload for QObject::connect." - You can just pass `ptr.get()` if you have a `unique_ptr` or `shared_ptr`. — Sebastian Redl, May 04 '19 at 18:09
@SebastianRedl Actually this post [Why don't the official Qt examples and tutorials use smart pointers?](https://stackoverflow.com/questions/34433435/why-dont-the-official-qt-examples-and-tutorials-use-smart-pointers) stopped me to do that and rethink about using smart pointers. — Const, May 04 '19 at 18:55
@mohabouje I am not quite sure about, QT's smart pointers. Rather I trust **`std::`smart pointers**. — Const, May 04 '19 at 18:56
@Const It's certainly a good idea to think twice about it. As mentioned in the answers, in your particular sample you don't need *any* kind of pointer. But if you do, the `get()` member is how you get a raw pointer out. Just don't let that raw pointer dangle by letting the smart pointer go out of scope. — Sebastian Redl, May 05 '19 at 06:08

score 3 · Accepted Answer · answered May 04 '19 at 17:32

Purpose of QPointer is to provide a guarded or weak pointer to a QObject subclass. It does not delete the object when it goes out of scope, it just knows if the object it points to is alive or deleted already.

So your current code is correct in that sense. A few comments:

It's not useful to have default value nullptr for the pointer in variable declaration, because you initialized it in the constructor initializer list.
You don't need to check if a pointer is null before deleting, because delete nullptr; is valid code which just does nothing.
If the lifetime of your QProcess is same as the lifetime of the containing object, then you should just put it as a member variable, and not use new at all, unless you have some specific reason.
If you do want to use new to allocate it, consider if the QProcess could have a parent QObject, which will delete it.
Alternatively, you should wrap the pointer in QScopedPointer or std::unique_ptr, because they own the object they point to and will delete it when they go out of scope.

Thanks for the review. Upped +1. I would prefer not to have pointers since it doesn't require here, in the current situation. — Const, May 04 '19 at 19:09

score 1 · Answer 2 · answered May 04 '19 at 17:29

QPointer is not a smart pointer. It does not manage the object it points to. It just tracks whether or not it was deleted. You will need to delete it yourself:

~MyClass()
{
    delete m_process.data();
}

When you delete the pointed-to object, data() will be null.

You don't need to check if it's null before deleting it, because deleting a null pointer is OK (it just doesn't do anything.)

I don't think you need a QPointer here though. As said before, this class is only useful for tracking whether or not the object has been deleted elsewhere. You should probably not be using a pointer at all here. Just do:

#include <QProcess>

class MyClass: public QObject {
private:
    QProcess m_process;
// ...

And change your connect code to:

connect(&m_process, &QProcess::errorOccurred, this, [](QProcess::ProcessError error)
{
    qDebug() << "error: " << error;
});

Thanks for the explanations. Upped +1. I would prefer not to have pointers since it doesn't require here, in the current situation. — Const, May 04 '19 at 19:09

Should I delete QPointer in the destructor of the class?

2 Answers2