Solved QList : dynamically add and delete objects
-
Hello,
I have a QList of Job (simple QObject derived class) objects
I need to add and remove Job objects dynamially in that QListI am not convinced of the correctness of my two methods
especially my delete Job methodplease have a look
QList<Job*> m_jobs; void addJob(QString _nom,QString _fullpath,float _xMax, float _yMax){ Job* job = new Job(this); if(!job){return;} job->setNom(_nom); // ... m_jobs.append(job); emit jobsChanged(); } //--- void deleteJob(QString pName, int pIndex){ if(pIndex<0||pIndex>m_selectedPrograms.length()){ qDebug()<< "can't delete job n° " << pIndex; return; } if(m_jobs.at(pIndex)->nom() == pName){ m_jobs.at(pIndex)->deleteLater(); // < m_jobs.removeAt(pIndex); // < emit jobsChanged(); } return; }
-
@LeLev Looks good, why do you think it could be wrong?
-
@LeLev
@jsulm has beaten me too it. Looks fine,QList
does not take ownership of things stored in it, you do your ownnew
&deleteLater
.The only tiny thing that is probably wrong is:
if(pIndex<0||pIndex>m_selectedPrograms.length()){
[You like squeezing every space out of your expressions to make them hard to read, do you? :) ] You probably intend
pIndex >= m_selectedPrograms.length()
[greater-than-or-equal] :) -
Looks all right.
Comments:
if(!job){return;}
unless you're catching the specific out of memory exception, this is pretty much superfluous. You could remove it.if(pIndex<0||pIndex>m_selectedPrograms.length()){
I'd simply assert here, i.e.:Q_ASSERT_X(pIndex < 0 || pIndex >= m_selectedPrograms.length(), "Can't delete job");
m_jobs.at(pIndex)
for brevity, you can retrieve the pointer once, although probably the optimizer is going to do that for you. Something like:Job * job = m_jobs.at(pIndex); // Qt asserts on out of bounds here anyhow if(job->nom() != pName) return; delete job; // You can use also deleteLater(). If you can guarantee there's nothing pending in the even queue for this object, delete is fine too. m_jobs.removeAt(pIndex); emit jobsChanged();
-
@LeLev seems, fine, as the others said,
just one point, since you seem to like to use as little text as possible :)
if(m_jobs.at(pIndex)->nom() == pName){ m_jobs.takeAt(pIndex)->deleteLater(); emit jobsChanged(); }
-
@kshegunov
Right, this is helpful, but you're always a stickler for detail, so I'm going to suggest something in your code :)delete job; // You can use also deleteLater(). If you can guarantee theres nothing pending in the even queue for this object, delete is fine too. m_jobs.removeAt(pIndex);
Why do you
delete
an object whilem_jobs
still contains a reference to it, which is invalid for a nanosecond? Why not reverse the lines for good coding practice:m_jobs.removeAt(pIndex); delete job;
I know I'm struggling here to find a real-world case, but if an "interrupt" arrives between these two statements and code "walks"
m_jobs
it won't be good. I'm going to put a debugger breakpoint after yourdelete
and before yourremoveAt()
, and get worried when I look atm_jobs
in the debugger ;-) -
thank you so much for your inputs friends ! really appreciate
@jsulm honestly it's the first time i really need create / delete objects dynamically in my c++ application. I'm not familiar (i will) with pointers and dynamic memory management..
-
@JonB said in QList : dynamically add and delete objects:
Why do you delete an object while m_jobs still contains a reference to it, which is invalid for a nanosecond? Why not reverse the lines for good coding practice:
Because no reason. It was written like this, and I wrote it in the same order. Since we are talking single-threaded here it really means absolutely nothing in what order you delete and remove. That's proper too, because you don't want code that's behaviorally dependent on order. If it were a multithreaded example, then this'd be serialized between the threads, so same considerations apply.
-
void deleteJob(QString pName, int pIndex){ Q_ASSERT_X(pIndex < 0 || pIndex >= m_jobs.length(), "deleteJob" ,"Can't delete job"); if( m_jobs.at(pIndex)->nom() == pName ){ m_jobs.takeAt(pIndex)->deleteLater(); emit jobsChanged(); } return; }
@kshegunov i'm little bit confused about Q_ASSERT_X
Q_ASSERT_X(pIndex < 0 || pIndex >= m_jobs.length(), "deleteJob" ,"Can't delete job"); here (if pIndex < 0) or (pIndex >= m_jobs.length()) we want to throw en error
or
Q_ASSERT_X(pIndex >= 0 && pIndex < m_jobs.length(), "deleteJob" ,"Can't delete job");
-
@LeLev said in QList : dynamically add and delete objects:
Q_ASSERT_X
Haven't you & @kshegunov got your test the wrong way round?? If the condition is false it will assert, i.e. it wants the condition to be true, just like
assert
. So you need:Q_ASSERT_X(pIndex >= 0 && pIndex < m_selectedPrograms.length(), "delete" ,"Can't delete job");
Right, or am I going mad?! :)
-
@JonB i was trying to figure this out right now,
what is strange is that both versions work.. i never see an error message printed.
I even tried
Q_ASSERT_X(pIndex ==1000 , "deleteProgram" ,"Can't delete job");Q_ASSERT_X(false, "deleteProgram" ,"Can't delete job");
and this must be false ! and it should print error message but this still works .. what is going on here how Q_ASSERT_X must work ?
-
@LeLev
Q_ASSERT_X(false, ...
fails the assertion and prints the message.
Q_ASSERT_X(true, ...
succeeds the assertion and continues."Assert" means "make sure this is true, else error". Same for
Q_ASSERT_X
,Q_ASSERT
&assert()
.And for the Qt ones
It does nothing if
QT_NO_DEBUG
was defined during compilation.You won't get anything at all in Release builds, regardless..... They're intended as debug development checks.
-
@JonB said in QList : dynamically add and delete objects:
Q_ASSERT_X(false, ... fails the assertion and prints the message.
in my case, right now, it is not..
i did
void deleteProgram(QString pName, int pIndex){ Q_ASSERT_X(false, "deleteProgram" ,"Can't delete job"); if( m_selectedPrograms.at(pIndex)->nom() == pName ){ m_selectedPrograms.takeAt(pIndex)->deleteLater(); emit selectedProgramsChanged(); } }
and program is deleted, no error printed
i will try clean/rebuild -
@LeLev
See what I've just written about Debug/Release builds! Same with underlyingassert()
etc. macros, usually (they check for a symbol likeDEBUG
or_DEBUG
, I think). -
@JonB said in QList : dynamically add and delete objects:
See
ok ! thank you i thought i was starting to get crazy..