Memory leak?

Creatorczyk

Hi,

I implement class like below, but I am not sure if there is a memory leak.

point.h

class Point : public QwtPlotMarker
{
public:
    explicit Point(const QColor &color);
    void setPosition(const double x, const double y, const bool status);

private:
    QwtSymbol *symbol;
    QColor color;
};

point.cpp

Point::Point(const QColor &color) :
    color(color)
{
    symbol = new QwtSymbol();
}

void Point::setPosition(const double x, const double y, const bool status)
{
    if(status){
        symbol = new QwtSymbol(QwtSymbol::XCross);
    }else{
        symbol = new QwtSymbol(QwtSymbol::Ellipse);
    }
    setSymbol(symbol);
    setValue(x,y);
}

Is the code ok? Function setPosition () is called multiple times

KroMignon

@Creatorczyk said in Memory leak?:

Is the code ok? Function setPosition () is called multiple times

No, this code is not okay, you newer release the created objects!
You have 3 ways to solve this:

add free to destroy old instances (and don't forget the destructor!!)
use smart pointers to not have to handle it yourself (cf. QSharedPointer())
no use pointer at all!

class Point : public QwtPlotMarker
{
public:
    explicit Point(const QColor &color): color(color)  {}
    void setPosition(const double x, const double y, const bool status)
    {
        symbol.setStyle (status ? QwtSymbol::XCross : QwtSymbol::Ellipse);
        ...
    }

private:
    QwtSymbol symbol;
    QColor color;
};

J.Hilk

@Creatorczyk said in Memory leak?:

Is the code ok?

no, its not. Its indeed leaking.

private:
    QwtSymbol *symbol = nullptr;

void Point::setPosition(const double x, const double y, const bool status)
{
    if(symbol)
        delete symbol;
    if(status){
        symbol = new QwtSymbol(QwtSymbol::XCross);
    }else{
        symbol = new QwtSymbol(QwtSymbol::Ellipse);
    }
    setSymbol(symbol);
    setValue(x,y);
}

Creatorczyk

@J-Hilk The application crashes with your method

J.Hilk

@Creatorczyk where? where does the debugger break? Whats the stack trace like ?

KroMignon

@Creatorczyk said in Memory leak?:

Is the code ok? Function setPosition () is called multiple times

No, this code is not okay, you newer release the created objects!
You have 3 ways to solve this:

add free to destroy old instances (and don't forget the destructor!!)
use smart pointers to not have to handle it yourself (cf. QSharedPointer())
no use pointer at all!

class Point : public QwtPlotMarker
{
public:
    explicit Point(const QColor &color): color(color)  {}
    void setPosition(const double x, const double y, const bool status)
    {
        symbol.setStyle (status ? QwtSymbol::XCross : QwtSymbol::Ellipse);
        ...
    }

private:
    QwtSymbol symbol;
    QColor color;
};

mpergand

Looking at QwtPlotMarker:: setSymbol:

void QwtPlotMarker::setSymbol( const QwtSymbol* symbol )
 {
     if ( symbol != m_data->symbol )
     {
         delete m_data->symbol;
         m_data->symbol = symbol;
  
         if ( symbol )
             setLegendIconSize( symbol->boundingRect().size() );
  
         legendChanged();
         itemChanged();
     }
 }

explain the crash.

~PrivateData()
     {
         delete symbol;
     }

QwtPlotMarker delete symbol in the destructor.

Conclusion: QwtPlotMarker do the right think,

private:
    QwtSymbol *symbol;

this variable of class Point must be seen as a 'weak pointer' that is, this object is managed by another object (here QwtPlotMarker)

J.Hilk

@mpergand oh, that explains a lot, deleting non 0 pointers twice is UB. the QwtPlotMaker does manage that memory.

Conclusion: QwtPlotMarker do the right think,

I would disagree that is not a good api design.

mpergand

@J-Hilk said in Memory leak?:

I would disagree that is not a good api design.

I fully agree with you.

I know nothing about QwtPlotMarker , but it must clearly be explained in the doc that setSymbol take ownership of symbol.
Yeah, it's bad design in C++.

KroMignon

@mpergand said in Memory leak?:

I know nothing about QwtPlotMarker , but it must clearly be explained in the doc that setSymbol take ownership of symbol.
Yeah, it's bad design in C++.

I never used Qwt, but for me, it look like you don't use it as it was designed...
Why do you have a QwtSymbol pointer in your Point class?
As QwtPlotMarker will take ownerchip of the instance, it is a dangerous class construction.