Solved Memory leak?
-
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
-
@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; };
- add
-
@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); }
-
@J-Hilk The application crashes with your method
-
@Creatorczyk where? where does the debugger break? Whats the stack trace like ?
-
@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; };
- add
-
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)
-
@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.
-
@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++. -
@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 aQwtSymbol
pointer in yourPoint
class?
AsQwtPlotMarker
will take ownerchip of the instance, it is a dangerous class construction.