CODEWITHSUNDEEP
c++qt
Unimportant
Unimportant

Reputation: 2096

QT Image Viewer example, possible memory leak?

A Image Viewer Example on the QT documentation website contains the following code snippet:

ImageViewer::ImageViewer()
   : imageLabel(new QLabel)
   , scrollArea(new QScrollArea)
   , scaleFactor(1)
{
   imageLabel->setBackgroundRole(QPalette::Base);
   imageLabel->setSizePolicy(QSizePolicy::Ignored, QSizePolicy::Ignored);
   imageLabel->setScaledContents(true);

   scrollArea->setBackgroundRole(QPalette::Dark);
   scrollArea->setWidget(imageLabel);
   scrollArea->setVisible(false);
   setCentralWidget(scrollArea);

   createActions();

   resize(QGuiApplication::primaryScreen()->availableSize() * 3 / 5);
}

Where imageLabel and scrollArea are pointers members to a QLabel and QScrollArea respectively.

I understand that in the line scrollArea->setWidget(imageLabel); the scroll area takes ownership of the imageLabel pointer and will delete it when required. Likewise for setCentralWidget(scrollArea); where the window takes ownership of the scrollArea.

However, during construction, if the imageLabel creation were to succeed, but the scrollArea creation were to fail, would the imageLabel not be leaked?

If yes, what is the canonical way to solve this?

Upvotes: 1

Views: 312

Answers (3)

HiFile.app - best file manager
HiFile.app - best file manager

Reputation: 7954

Well, this is probably overall a problem of Qt because it is not completely exception safe https://doc.qt.io/qt-5/exceptionsafety.html

From practical point of view it does not matter unless you create mission critical apps. If the construction of scroll area (i.e. dynamic allocation) fails, then this means there is something really terrible going on: all your memory is exhausted and not even swapping memory to disk helps solve it. Some memory leak is your least concern in such a situation. You are probably going to crash or complete freeze your computer anyway and restart will be necessary.

Of course, you can try to solve partial piece of code with smart pointers but it still probably leaves lots of leaks inside Qt library itself. Your program will malfunction anyway if you completely run out of memory.

So unless you create really bullet-proof mission critical apps, your probably do not need to care about such situation. If I were you, I would care more about systematic memory leaks, where you forget to release memory under NORMAL circumstances.

Unless you create mission critical apps, you do not need to worry about running out of memory. It is a "vis maior" and you can hardly do anything to protect your app when this situation comes. The good news is that most of us never experience such a situation under normal circumstances.

Upvotes: 0

Unimportant
Unimportant

Reputation: 2096

Solved it like this for now:

ImageViewer::ImageViewer()
    : scaleFactor(1)
{
   //Manage pointers with unique_ptr until it's time to transfer ownership. 
   auto uniqueImageLabel = std::make_unique<QLabel>();
   auto uniqueScrollArea = std::make_unique<QScrollArea>();

   //Copy pointers to members for access as per normal.
   imageLabel = uniqueImageLabel.get();
   scrollArea = uniqueScrollArea.get(); 

   imageLabel->setBackgroundRole(QPalette::Base);
   imageLabel->setSizePolicy(QSizePolicy::Ignored, QSizePolicy::Ignored);
   imageLabel->setScaledContents(true);

   scrollArea->setBackgroundRole(QPalette::Dark);
   //ScrollArea now takes ownership of image label. 
   scrollArea->setWidget(uniqueImageLabel.release());
   scrollArea->setVisible(false);
   //Window now takes ownership of scroll area.
   setCentralWidget(uniqueScrollArea.release());

   createActions();

   resize(QGuiApplication::primaryScreen()->availableSize() * 3 / 5);
}

Upvotes: 0

Andrii
Andrii

Reputation: 1906

I think there are two ways to go with:

1. Use smart pointers instead of raw (e.g. QPointer).

    class ImageViewer : public QMainWindow
    {
       QPointer<QLabel> imageLabel;
       QPointer<QScrollArea> scrollArea;
    };

In this case imageLabel's destructor will be called if scrollArea's constructor (or ImageViewer constructor's body thrown an exception)

2. Move a memory allocation inside a constructor's body and wrap by try/catch block.

    ImageViewer::ImageViewer()
        : imageLabel(nullptr)
        , scrollArea(nullptr)
        , scaleFactor(1)
    {
        try {
            imageLabel = new QLabel();
            scrollArea = new QScrollArea();
        } catch (std::bad_alloc&) {
            delete imageLabel;
            delete scrollAreal;
        }
        // ...
    }

More details might be found here (Moral #4 is about your question)

Upvotes: 1

Related Questions