Important: Please read the Qt Code of Conduct - https://forum.qt.io/topic/113070/qt-code-of-conduct

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

  • Lifetime Qt Champion

    @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 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] :)


  • Qt Champions 2017

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

  • Moderators

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



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


  • Qt Champions 2017

    @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 underlying assert() etc. macros, usually (they check for a symbol like DEBUG 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..


Log in to reply