2

So I have the following piece of code written in Qt C++:

// Post* derives from QObject
Post* post = new Post(this);
QString url;

ParseResult result = parse(url, post); // this function takes a Post* and modifies it

// if result is succesfully parsed
if(result == ParseResult::Success){
    // add the post to a std::vector<Post*> which is a member of the class
}
else{
    post->deleteLater();
}

Now as you can see I am deleting the raw pointer Post*, however I was wondering if what I have above could be implemented in another way which wouldn't require the use of deleteLater().

reckless
  • 741
  • 12
  • 53
  • 1
    Does _smart pointers_ count as an answer? – nada Aug 13 '19 at 16:19
  • 4
    @nada Not with Qt. You can get double deletion (once by the smart pointer, once by the QObject's parent.) – Nikos C. Aug 13 '19 at 16:19
  • @nada sure if the smart pointer solution is compatible with Qt signal and slots system, but I though that wasn't possible – reckless Aug 13 '19 at 16:20
  • 1
    Then my answer is: _Fix Qt to be compatible with smart pointers_ and after that _use smart pointers_. – nada Aug 13 '19 at 16:22
  • Relevant: https://agateau.com/2019/using-std-unique-ptr-with-qt/ – Nikos C. Aug 13 '19 at 16:23
  • @nada That's not possible. You cannot do that without a complete redesign of Qt. At that point, you might as well create your own separate framework. – Justin Aug 13 '19 at 16:25
  • `unique_ptr` with a custom deleter which calls `deleteLater`? – Alan Birtles Aug 13 '19 at 16:37
  • 2
    @AlanBirtles That's considered over-engineering, at least by the author of this answer - [Why don't the official Qt examples and tutorials use smart pointers?](https://stackoverflow.com/a/34434321/1241334) – Jonny Henly Aug 13 '19 at 16:38
  • 2
    @JonnyHenly The answer you linked to misses the point. Smart pointers are not just about leaks. They're also about exception safety and making mistakes less likely. Doing it the "Qt way" requires you to do it correctly. But we're humans and so we can make mistakes. If you make a mistake, you can leak (for example you allocate an object with no parent, but between the allocation and the point where you assign it a parent something happens and you exit the scope.) With smart pointers, such mistakes are more difficult, and thus there's less room for bugs. – Nikos C. Aug 13 '19 at 17:17

1 Answers1

3

If post is not going to outlive this, and is always going to be a child of this (meaning it will never get reparented) then you can store the instances in a vector of unique_ptr<Post>:

class This: /* ... */ {
// ...
    std::vector<std::unique_ptr<Post>> post_vec_;
};

Now your code can omit the deletion:

auto post = std::make_unique<Post>(this);
QString url;

ParseResult result = parse(url, post.get());
if (result == ParseResult::Success) {
    post_vec_.push_back(std::move(post));
    // 'post' was moved from, so it's just a nullptr now.
}

// No 'else' needed. 'post' is going to be deleted when it goes out of scope.

(This assumes that post->deleteLater() could have been replaced with delete post in your original code. It doesn't seem like you have a reason to use deleteLater() rather than just calling delete directly.)

When this is destroyed, its members are going to be destroyed first before calling the base class destructor. So post_vec_ is going to get destroyed first, which means the smart pointers it contains are going to delete the Post objects they manage. The destructors of those objects are going to unregister the objects from their parents, so no double-deletion can happen. This is why this answer has the requirement that these objects must not outlive this and must not get reparented.

As a final note, if you don't want to expose the Post class in the This header, you can just forward declare it:

class Post;

class This: /* ... */ {
// ...
    std::vector<std::unique_ptr<Post>> post_vec_;
};

unique_ptr can still work with that, as long as you implement your destructor in the .cpp file. If you don't have a destructor, then you can use the default one, but you need to default it in the .cpp file:

class Post;

class This: /* ... */ {
public:
    // ...
    ~This() override;

// ...
    std::vector<std::unique_ptr<Post>> post_vec_;
};

In the .cpp file:

This::~This() = default;
Nikos C.
  • 50,738
  • 9
  • 71
  • 96
  • 1
    If you use `QObject` inside smart pointer it is better to use `QPointer` instead `std::unique_ptr` – Serhiy Kulish Aug 13 '19 at 17:48
  • @SerhiyKulish `QPointer` does not automatically delete the managed object, which is what the OP wants here. It just sets itself to null when someone else deletes the object. See: https://doc.qt.io/qt-5/qpointer.html#dtor.QPointer – Nikos C. Aug 13 '19 at 17:51