QList and removeAt() question

[ally]

Hi; I have a little question about QList and memory usage. On my header I have a QList pointer:

QList<MyObjectData> *myList;

QList<MyObjectData> *myList;

To copy to clipboard, switch view to plain text mode 

on class an init method

void MyClass::init(){
 
myList= new QList<MyObjectData>;
 
}

void MyClass::init(){

myList= new QList<MyObjectData>;

}

To copy to clipboard, switch view to plain text mode 

periodically I feed my list with new elements :

void MyClass::feedList(int id){
 
MyObjectData mod;
mod.setId(id);
myList->append(mod);
 
}

void MyClass::feedList(int id){

MyObjectData mod;
mod.setId(id);
myList->append(mod);

}

To copy to clipboard, switch view to plain text mode 

and method called by a timer take this data, use them and removing by calling removeAt(0) :

void MyClass::useData(){
 
if(commandRequest->size()>0 && busy == false)
	{
		busy = true;
		executeSomething((MyObjectData )myList->at(0));
		myList->removeAt(0);
		busy = false;
	}
}

void MyClass::useData(){

if(commandRequest->size()>0 && busy == false)
	{
		busy = true;
		executeSomething((MyObjectData )myList->at(0));
		myList->removeAt(0);
		busy = false;
	}
}

To copy to clipboard, switch view to plain text mode 

can be any memory issue whit these methods? thank you...

[jefftee]

You don't show where you delete myList in your code. You should delete in init's counterpart (cleanup?) or your MyClass destructor so that you don't leak memory for the list itself. I would add the following code to do that:

myList.clear(); // done by destructor, but doesn't hurt to clear IMHO
delete myList;
myList = nullptr;

myList.clear(); // done by destructor, but doesn't hurt to clear IMHO
delete myList;
myList = nullptr;

To copy to clipboard, switch view to plain text mode 

Since you are not storing pointers to objects in your list, I don't see any issues with your handling of MyObjectData.

P.S. You will crash if your myList is empty when you run this code, so also check that myList->size() > 0 in your if statement.

[anda_skoa]

myList.clear();
delete myList; // some people prefer myList->deleteLater() but my pref is to control alloc/dealloc for object instances I manage.
myList = NULL;

myList.clear();
delete myList; // some people prefer myList->deleteLater() but my pref is to control alloc/dealloc for object instances I manage.
myList = NULL;

To copy to clipboard, switch view to plain text mode 

You don't need to clear() explicitly, the destructor does that.
QList does not have a deleteLater() method, it is not a QObject subclass.
The value of a null pointer is either 0 or nullptr

Since you are not storing pointers to objects in your list, I don't see any issues with your handling of MyObjectData.

Indeed

P.S. You will crash if your myList is empty when you run this code, so also check that myList->size() > 0 in your if statement.

And there is no need for the cast, QList::at() returns the correct type itself.

Cheers,
_

[jefftee]

You don't need to clear() explicitly, the destructor does that.
QList does not have a deleteLater() method, it is not a QObject subclass.
The value of a null pointer is either 0 or nullptr

Thanks for correcting my post @anda_skoa. For that null pointer assignment, old habits are hard to break! I will update my post so as not to mislead anyone that runs across this in the future.

[ally]

thank you. In userData method there's already control of array lenght but I forget to rename QList when I post it on forum :

//commandRequest is the real name of my QList, I call it "myList" only in this post
if([B]commandRequest[/B]->size()>0 && busy == false)
	{
		busy = true;
		executeSomething((MyObjectData )myList->at(0));
		myList->removeAt(0);
		busy = false;
	}
}

//commandRequest is the real name of my QList, I call it "myList" only in this post
if([B]commandRequest[/B]->size()>0 && busy == false)
	{
		busy = true;
		executeSomething((MyObjectData )myList->at(0));
		myList->removeAt(0);
		busy = false;
	}
}

To copy to clipboard, switch view to plain text mode 

[jefftee]

Ok, understood, but you'll get better responses when you provide actual code rather than dummy code for posting to forums. All too often people post code that won't compile or isn't actually the code they're using, or omit something they think isn't relevant, which can obscure the real problem, etc.

BTW, what is your objective in using the busy boolean the way you show in the code?

If your application isn't multi-threaded, then the busy boolean isn't needed because it's not protecting anything and if your code is actually multi-threaded and the QList is accessed by multiple threads concurrently, you should serialize access to the QList by locking/unlocking with a QMutex or use the QMutexLocker convenience class.

[ally]

Ok thank you but I change only method's name. UseData method is periodically call by a QTimer so I decide to use boolen to prevent multiple launch when executeSomething(MyObjectData) method require more time than QTimer. Useless?

[anda_skoa]

QTimer is event based.
As long as your slot is executed, no further events can be processed.
The timer can only fire again after execution has returned to the event loop.

Cheers,
_

[ally]

ah ok, thank you