PDA

View Full Version : Ownership of the QPointf that are created with new appended in QList<QPointf> list



Ahmed Abdellatif
14th September 2018, 14:10
I have a QList<QPOintf> that has 10 element inside it, i want to copy the list in a reverse order then append it again to the list but this is done after changing the y value of QPointf. For example:

suppose our list is as follow:

m_resultPoints has the following points [(1,2),(1,3),(5,6),(7,2),(3,4),(7,9),(4,5),(2,0),(2 ,4),(4,7)];
I want it to be:

m_resultPoints should have the following points [(1,2),(1,3),(5,6),(7,2),(3,4),(7,9),(4,5),(2,0),(2 ,4),(4,7),(4,-7),(2,-4),(2,-0),(4,-5),(7,-9),(3,-4),(7,-2),(5,-6),(1,-3),(1,-2)];

So I put the list in a loop and created a QPointf dynamically using new operator and appended it to the list content. Now who take the owner ship of the points, i think it is the QList, am i right ?
I also wonder if there is another way to do this in more elegant way, thanks in advance.

Here is the code snippet that is used to do the appending and the reversing of the list points:

for (int i = 0; i < m_resultPoints->size(); ++i) {
m_resultPoints->append(*new QPointF(m_resultPoints->at(m_resultPoints->size()-i).x() , (-1*m_resultPoints->at(m_resultPoints->size()-i).y() )));
}

ChristianEhrlicher
14th September 2018, 16:40
Why do you create the QPointF on the heap instead just passing the QPointF to the QVector - then you don't have to clean up the memory when removing the points from the vector. And it's faster too...

tuli
14th September 2018, 17:10
Literally everything about this is wrong. You need to learn C++ first.


* if you do '*new QPointf...' you are leaking memory. You first create a QPointof on the heap, then deference it with *. This creates a copy of the allocated QPointf.

* Dont allocate it on the heap with new at all. Just do QPointf(1,1).

* resultPoints->size() is the current size, if you append to the list, the size grows. Hence the loop will go on forever.

Without having tried it, more like so:


QList<QPointF> m_resultPoints;
int size = m_resultPoints.size();
for(int i=0; i<size; i++)
{
points.append( QPointF(m_resultPoints->at(size-i).x() , (-1*m_resultPoints->at(size-i).y() )
}

d_stranz
14th September 2018, 17:19
Now who take the owner ship of the points, i think it is the QList, am i right ?

No, the list does not take ownership. QList is not a QObject-based class, so there is no parent-child or other type of ownership relationship.

I think you also misunderstand the difference between QList< QPointF > and QList< QPointF * >. As a result your code has a memory leak.

You create QPointF instances on the heap using operator new(), and then pass a reference (*pointf) into the QList, which then makes a copy of the QPointF's contents. The QPointF pointer you created becomes an orphan pointer - the memory is still allocated, but you have no way to free it because the pointer to it is only accessible for the life of the append call. So every time you call append, a little bit more heap gets eaten away and is never available again until your program exits.

The way around this is to not create the pointer in the first place, as Christian says. Your code should be changed to this:



for (int i = 0; i < m_resultPoints->size(); ++i)
{
m_resultPoints->append( QPointF(m_resultPoints->at(m_resultPoints->size()-i).x() , (-1*m_resultPoints->at(m_resultPoints->size()-i).y() )));
}


Look carefully at the argument to the append() call and you will see the difference. In this case, the QPointF instances are being allocated on the stack (not the heap) and will automatically be destroyed when the QList is destroyed.

Edit: I see tuli and I posted at the same time. His comments are correct, too. I hadn't noticed that the resultPoints->size() call was inside the loop, and it will result in worse than just a memory leak. It will cause your program to run in an infinite loop until it crashes because it has run out of memory. If you are running on a 64-bit computer, it will eventually bring your computer to a halt too as the program keeps trying to allocate more scratch memory from the disk-based heap.