QList : dynamically add and delete objects

ODБOï

Hello,
I have a QList of Job (simple QObject derived class) objects
I need to add and remove Job objects dynamially in that QList

I am not convinced of the correctness of my two methods
especially my delete Job method

please 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;
}

jsulm

@LeLev Looks good, why do you think it could be wrong?

JonB

@LeLev
@jsulm has beaten me too it. Looks fine, QList does not take ownership of things stored in it, you do your own new & 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] :)

kshegunov

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();

J.Hilk

@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();
    }

JonB

@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 while m_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 your delete and before your removeAt(), and get worried when I look at m_jobs in the debugger ;-)

ODБOï

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..

kshegunov

@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.

ODБOï

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");

JonB

@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?! :)

ODБOï

@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 ?

JonB

@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.

ODБOï

@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

JonB

@LeLev
See what I've just written about Debug/Release builds! Same with underlying assert() etc. macros, usually (they check for a symbol like DEBUG or _DEBUG, I think).

ODБOï

@JonB said in QList : dynamically add and delete objects:

See

ok ! thank you i thought i was starting to get crazy..