Recurring C++ and Qt anti-patterns



  • @aha_1980 said in Recurring C++ and Qt anti-patterns:

    because delete does the check anyway

    Wait, what? Since when does C++ check if the pointer is nullptr before delete? I didn't know this was a thing now.

    Also, is deleteLater not a way to delete Qt objects? Its morning and I haven't had my coffee.



  • @fcarney said in Recurring C++ and Qt anti-patterns:

    Wait, what? Since when does C++ check if the pointer is nullptr before delete?

    Wow, since 2003? Lol, keep this thread going! I am learning a lot.



  • Okay, I am just confusing myself. If you delete a pointer you must immediately set it to null. Otherwise you risk double delete, which is bad. But its okay to delete something set to null. Got it.

    Edit:
    Why doesn't delete set the pointer to null then? That seems like it may be an antipattern in and of itself.


  • Qt Champions 2018

    @fcarney said in Recurring C++ and Qt anti-patterns:

    Why doesn't delete set the pointer to null then? That seems like it may be an antipattern in and of itself.

    I have indeed asked that myself. If someone has the correct answer for that, I'm all ears.



  • @aha_1980
    Apparently the standard allows for it:
    https://stackoverflow.com/questions/704466/why-doesnt-delete-set-the-pointer-to-null

    The creator himself wonders why it isn't so. Its like C++ is this beautiful, amazing, and now, WILD animal roaming free in cyberspace... Yeah, maybe the analogy isn't all that great, but it does conjure up a cool picture.



  • @fcarney said in Recurring C++ and Qt anti-patterns:

    Why doesn't delete set the pointer to null

    likely because it would bring more problems than solutions


  • Qt Champions 2018

    @LeLev

    likely because it would bring more problems than solutions

    That would mean, that this pointer shows to an invalid memory region after the delete. Can you think of an example where you still want to use that pointer afterwards? (That is a real question - because for now I have no idea).



  • @LeLev said in Recurring C++ and Qt anti-patterns:

    likely because it would bring more problems than solutions

    I could see a case where a program is deleting thousands of pointers and there might actually be overhead in a mov instruction for each delete. I have no idea if this overhead would be significant over the delete operation, but it would still be overhead. It would not be that hard to test such a scenario. I should try it!



  • @aha_1980 said in Recurring C++ and Qt anti-patterns:

    Can you think of an example where you still want to use that pointer afterwards?

    not a real world application



  • @fcarney said in Recurring C++ and Qt anti-patterns:

    deleting thousands of pointers and there might actually be overhead

    I cannot actually tell if the overhead in this code is the indexing of the array, or if the movement of data is significant. I tried doing a dummy no op index, but I am guessing it is being optimized out:

    #include <QCoreApplication>
    #include <QElapsedTimer>
    #include <QDebug>
    
    #define MEM_SEG_LEN 8
    #define MEM_SEGS 100000000
    
    char** createMemoryList(){
        char** list = new char*[MEM_SEGS];
        for(int index=0; index<MEM_SEGS; index++){
            list[index]=new char[MEM_SEG_LEN];
        }
    
        return list;
    }
    
    void deleteMemoryList(char** list){
        for(int index=0; index<MEM_SEGS; index++){
            delete list[index];
            list[index]; // can you force an index to occur?
        }
        delete list;
    }
    
    void deleteMemoryListNull(char** list){
        for(int index=0; index<MEM_SEGS; index++){
            delete list[index];
            list[index] = nullptr;
        }
        delete list;
        list = nullptr;
    }
    
    int main(int argc, char *argv[])
    {
        QCoreApplication a(argc, argv);
    
        QElapsedTimer timer1;
    
        char** list1 = createMemoryList();
        timer1.start();
        deleteMemoryList(list1);
        qInfo() << timer1.elapsed();
    
        QElapsedTimer timer2;
    
        char** list2 = createMemoryList();
        timer2.start();
        deleteMemoryListNull(list2);
        qInfo() << timer2.elapsed();
    
        return a.exec();
    }
    

    I get the following output:

    813
    1301
    

    I doubt that is the overhead of the movement of null into the pointer. My guess is the the index overhead is in there too.



  • I eliminated the extra index (probably compiler already did this before):

    void deleteMemoryList(char** list){
        for(int index=0; index<MEM_SEGS; index++){
            char* tmp = list[index];
            delete tmp;
        }
        delete list;
    }
    
    void deleteMemoryListNull(char** list){
        for(int index=0; index<MEM_SEGS; index++){
            char* tmp = list[index];
            delete tmp;
            tmp = nullptr;
        }
        delete list;
        list = nullptr;
    }
    

    Results:

    877
    1369
    

    Edit: Real world usage? I really highly doubt it. That is a LOT of iterations of delete. So I would say the extra cycles are negligible.

    Edit2:
    Pointer math:

    void deleteMemoryList(char** list){
        for(int index=0; index<MEM_SEGS; index++){
            char** tmp = &(list[index]);
            delete *tmp;
        }
        delete list;
    }
    
    void deleteMemoryListNull(char** list){
        for(int index=0; index<MEM_SEGS; index++){
            char** tmp = &(list[index]);
            delete *tmp;
            *tmp = nullptr;
        }
        delete list;
        list = nullptr;
    }
    

    Results:

    853
    1307
    

    Sometimes apples and apples is hard.



  • @fcarney
    Since this is the lounge... Surprised by your findings (in earlier examples). What exactly is the difference in the assembly between the two versions? What is being generated for your tmp = nullptr;? (Not the later *tmp = nullptr;, that's different.)



  • @JonB said in Recurring C++ and Qt anti-patterns:

    tmp = nullptr;

    I changed it to not update a local variable. char* tmp is local, so setting it to null is just setting a local variable to null. So it was setting the wrong area of memory to null. That is why I took the address of where that pointer is stored.

    The assembler for tmp = nullptr in previous incarnation:

    movq   $0x0,-0x8(%rbp)
    

    The assembler for *tmp = nullptr in latest incarnation:

    mov    -0x8(%rbp),%rax
    movq   $0x0,(%rax)
    

    But you are right, the tmp = nullptr is more representative.
    The timing is not much different.



  • Okay, I think I am done, but here is my last incarnation:

    #include <QCoreApplication>
    #include <QElapsedTimer>
    #include <QDebug>
    
    #define MEM_SEG_LEN 8
    #define MEM_SEGS 100000000 // 846 1315 // assignment
    #define MEM_SEGS 100000000 // 885 1349 // correct assignment
    //#define MEM_SEGS 100
    
    char** createMemoryList(){
        char** list = new char*[MEM_SEGS];
        for(int index=0; index<MEM_SEGS; index++){
            list[index]=new char[MEM_SEG_LEN];
        }
    
        return list;
    }
    
    void deleteMemoryList(char** list){
        for(int index=0; index<MEM_SEGS; index++){
            char** tmp = &(list[index]);
            delete *tmp;
        }
        delete list;
    }
    
    void deleteMemoryListNull(char** list){
        for(int index=0; index<MEM_SEGS; index++){
            char** tmp = &(list[index]);
            delete *tmp;
            *tmp = nullptr;
    //        char* tmp = (list[index]);
    //        delete tmp;
    //        tmp = nullptr;
        }
        delete list;
        list = nullptr;
    }
    
    int main(int argc, char *argv[])
    {
        QCoreApplication a(argc, argv);
    
        double time1, time2;
    
        qInfo() << QString(R"(Starting memory test 1 of not setting pointer to nullptr: %1 cycles)").arg(MEM_SEGS);
    
        QElapsedTimer timer1;
    
        char** list1 = createMemoryList();
        timer1.start();
        deleteMemoryList(list1);
        time1 = timer1.elapsed();
        qInfo() << time1/1000.0 << QString().number((time1/1000.0)/double(MEM_SEGS),10,12);
    
        qInfo() << QString(R"(Starting memory test 2 of setting pointer to nullptr: %1 cycles)").arg(MEM_SEGS);
    
        QElapsedTimer timer2;
    
        char** list2 = createMemoryList();
        timer2.start();
        deleteMemoryListNull(list2);
        time2 = timer2.elapsed();
        qInfo() << time2/1000.0 << QString().number((time2/1000.0)/double(MEM_SEGS),10,12);
    
        qInfo() << "Difference:" << QString().number((time2/1000.0)/double(MEM_SEGS)-(time1/1000.0)/double(MEM_SEGS),10,12);
    
        return a.exec();
    }
    

    So, 5 nanoseconds difference for a delete operation of dereffed pointer assignment:

    "Starting memory test 1 of not setting pointer to nullptr: 100000000 cycles"
    0.848 "0.000000008480"
    "Starting memory test 2 of setting pointer to nullptr: 100000000 cycles"
    1.321 "0.000000013210"
    Difference: "0.000000004730"
    

    Edit:
    Math was wrong.



  • @fcarney
    You're talking about a single movq. Just how big is MEM_SEGS? Your timings, are they in milliseconds?? And I assume list is filled with zeroes? I'm talking about your earlier barebones example, where your timings were
    877
    1369

    Oh now I see more code in other examples. If your list contained 10 million news and you are deleteing them, common-sense should tell you the cost of whether or not you set one local variable to nullptr with just a mov instruction must be negligible, compared to whatever is involved in freeing memory, no?



  • @JonB said in Recurring C++ and Qt anti-patterns:

    one local variable to nullptr with just a mov instruction must be negligible, compared to whatever is involved in freeing memory, no?

    Correct. My math was wrong. It is 5 nanoseconds. At least for dereffed pointer, but timing was nearly the same for local variable as well.



  • @fcarney
    Ahhh, that would make much more sense! :)
    A nanosecond doesn't sound too long, I don't think I could get much done in it could I?


  • Qt Champions 2018

    One more example for the hall of shame:

    QByteArray ba = "Hello World";
    QString s = QString::fromStdString(ba.toStdString());
    

    We have two problems here:

    1. The conversion to and from std::string is unneeded
    2. This only works for ASCII characters. QString::fromUtf8(ba); would most often be the correct choice, sometimes also QString::fromLocal8Bit(ba);

  • Qt Champions 2018

    One more note to string processing:

    QString empty = ""; is not the correct way to create an empty string, that is QString empty;

    To clear an non-empty QString data, use data.clear() instead data = "";.

    The same applies to QByteArrays.



  • @aha_1980 said in Recurring C++ and Qt anti-patterns:

    QString empty = ""; is not the correct way to create an empty string, that is QString empty;
    To clear an non-empty QString data, use data.clear() instead data = "";

    Interesting... I did a couple of tests to see if it behaves the same way for comparisons. The only difference I can find between clear and setting to "" is the isNull test:

        qInfo() << "string test";
        QString empty;
        QString str = "";
        qInfo() << bool(empty == str);
        qInfo() << str.isNull();
        str.clear();
        qInfo() << bool(empty == str);
        qInfo() << str.isNull();
    

    output:

    string test
    true
    false
    true
    true
    

    If the string was set to "" is returns false for isNull. I have never had the occasion to use this test and am not sure what it is for. I guess it would be important if you want to determine if an empty string was assigned to the variable versus nothing being assigned.


  • Qt Champions 2018

    @fcarney I just think the isNull() method should be deprecated. Even the documentation states:

    Qt makes a distinction between null strings and empty strings for historical reasons. For most applications, what matters is whether or not a string contains any data, and this can be determined using the isEmpty() function.


  • Qt Champions 2018

    @fcarney said in Recurring C++ and Qt anti-patterns:

    Why doesn't delete set the pointer to null then?

    Because the operator delete is defined to take a pointer, not a reference to a pointer.
    https://en.cppreference.com/w/cpp/memory/new/operator_delete



  • Welcome to another edition of Is this an Anti-Pattern?

    Harmless looking pattern 1:

    std::vector<int64_t> list = {9,9,9,9,9,9,9,9,9,9};
    int64_t sum = std::accumulate(list.begin(), list.end(), 0);
    

    Harmless looking pattern 2:

    std::vector<int64_t> list = {9,9,9,9,9,9,9,9,9,9};
    int64_t prod = std::accumulate(list.begin(), list.end(), 1, std::multiplies<int64_t>());
    

    Lack of understanding pattern 3:

    std::set<int> list = {9,8,7,6};
    int last = *(list.end()--);
    

    Lack of understanding pattern 4:

    std:vector<int> list = {9,8,7,6};
    int last = *(list.end()--);
    

    I still don't get why this cannot be done for sets and vectors...
    Okay, was using post decrement instead of a prefix decrement. DOH!

    int last = *(--list.end());
    

    Logic fudgery pattern 5 (this is more an optical illusion, it was to me anyway):

    bool is_leap_year(int year){
        return (year % 4 == 0) && (year % 100 == 0) ? (year % 400 == 0) : true;
    }
    

  • Moderators

    Small things from me:

    • lack of const in signal declarations. A signal will never modify an object so it can always be const. Qt will const_cast it away anyway, but it enables you to emit signals from const methods and (possibly) compiler to optimize a bit more
    • overuse of lambdas in slot connections even when a normal slot just makes more sense


  • @sierdzio said in Recurring C++ and Qt anti-patterns:

    Small things from me:

    • lack of const in signal declarations. A signal will never modify an object so it can always be const. Qt will const_cast it away anyway, but it enables you to emit signals from const methods and (possibly) compiler to optimize a bit more
    • overuse of lambdas in slot connections even when a normal slot just makes more sense

    This brings me to a philosophical question: Do I want to be able to emit a signal from a const method, although the slot(s) attached to the signal may well modify data the originating const method could not itself modify?


  • Moderators

    @Asperamanca said in Recurring C++ and Qt anti-patterns:

    This brings me to a philosophical question: Do I want to be able to emit a signal from a const method, although the slot(s) attached to the signal may well modify data the originating const method could not itself modify?

    Yes, it's very debatable :D I did find a few occasions where it was useful (latest example: modifying behaviour of QTreeView without patching Qt - I have emitted a signal from const overloaded method and did my modifications there), but I agree it does not feel "right".



  • Given the code:
    modules.h

    #ifndef MODULES_H
    #define MODULES_H
    
    #include <string>
    
    void reg_module(int type, std::string name, int initedValue);
    
    #endif // MODULES_H
    

    modules.cpp

    #include "modules.h"
    
    using namespace std;
    
    struct Modules
    {
        Modules(): m_initedValue(0){}
        int m_type;
        string m_name;
        int m_initedValue;
    } global_modules_struct[128];
    
    void reg_module(int type, std::string name, int initedValue){
        global_modules_struct[type].m_type = type;
        global_modules_struct[type].m_name = name;
        global_modules_struct[type].m_initedValue = initedValue;
    }
    

    moduletype.h

    #ifndef MODULETYPE_H
    #define MODULETYPE_H
    
    // nothing here
    
    #endif // MODULETYPE_H
    

    moduletype.cpp

    #include "moduletype.h"
    #include "modules.h"
    
    struct SomeModule{
        SomeModule(){
            reg_module(10, "some type", 5);
        }
    } SomeModuleInstance;
    

    Ignore obvious indexing bounds checking issues for the array itself. Also ignore external array indexing possibly being out of bounds.

    I just ran into a form of this problem in our code and it did not exhibit issues in Linux (that we know of) and did show issues in Windows. Linux used gcc and Windows used mingw. Same version of Qt 5.12.2 etc. Once identified it was really easy to see why this is a big issue.

    Edit:
    Technically global_modules_struct is not really global either. So ignore the misleading name.


  • Moderators

    I had to dig through this thing once, only the real code was like a hundred times longer and more convoluted.

    // Library.h statically linked to and included in DLL and EXE
    struct SomeType
    {
       int typeId();
    };
    Q_DECLARE_METATYPE(SomeType);
    
    // Library.cpp
    int SomeType::typeId()
    {
        return qMetaTypeId<SomeType>();
    }
    
    // main app
    SomeType& var1 = getItFromDLL();
    SomeType& var2 = getItFromEXE();
    
    bool same = var1.typeId() == var2.typeId(); // nope
    

    Pretty ugly thing to debug, especially since once in blue moon it actually works :/


  • Banned

    This post is deleted!


  • @sierdzio said in Recurring C++ and Qt anti-patterns:

    Yes, it's very debatable :D I did find a few occasions where it was useful (latest example: modifying behaviour of QTreeView without patching Qt - I have emitted a signal from const overloaded method and did my modifications there), but I agree it does not feel "right".

    Actually, I have zero problem with this. The way my mind works it makes perfect sense, as the signal is a message to a receiving class (any class). It's not the sender method that modifies the object state. It is the message. My mind differentiates between the two.



  • @fcarney said in Recurring C++ and Qt anti-patterns:

    Apparently the standard allows for it:
    https://stackoverflow.com/questions/704466/why-doesnt-delete-set-the-pointer-to-null
    The creator himself wonders why it isn't so. Its like C++ is this beautiful, amazing, and now, WILD animal roaming free in cyberspace... Yeah, maybe the analogy isn't all that great, but it does conjure up a cool picture.

    Jumping back a few months on this one, but I think the decision to leave alone the pointer value upon an object delete is solid. If I understand the standard properly, the target of a delete can be an lvalue or and rvalue. So delete 0x34fc3d2200 should be a valid operation, right? How ya gonna change the value of an rvalue (in a traditional sense)?


  • Moderators

    Imagine clearing some sort of array:

    for(type* ptr = some_array; something ; ++ptr)
    {
         delete ptr;
    }
    

    Now imagine delete would zero that pointer. Do you see the problem? You would have to make another, temporary, pointer just so you zero the copy and your original doesn't get changed. In other words you're paying for what you don't use or even want. There's also problem of const pointers or pointers that you got from external APIs that do their own bookkeeping and might actually need that pointer value even after delete. It would create more problems than it solves.



  •         int 🥩=1;
            int 🧀=1;
            int 🥬=1;
            int 🍞=1;
            int 🍅=1;
            int 🥪=🥩+🥬+🍅+🧀+🥩;
            cout << 🥪 << endl;
    

    Fails to compile in C++17...



  • @fcarney said in Recurring C++ and Qt anti-patterns:

        int 🥩=1;
        int 🧀=1;
        int 🥬=1;
        int 🍞=1;
        int 🍅=1;
        int 🥪=🥩+🥬+🍅+🧀+🥩;
        cout << 🥪 << endl;
    

    Fails to compile in C++17...

    What's this "int" stuff? Doesn't the 17 standard deduce the type based on the rvalue? Not that I think that is necessarily a good thing though.



  • @kent-dorfman said in Recurring C++ and Qt anti-patterns:

    🍞

    The real problem is this variable is unused.



  • @fcarney said in Recurring C++ and Qt anti-patterns:

    The real problem is this variable is unused.

    So in 17 unused variables are errors instead of warnings?



  • @kent-dorfman said in Recurring C++ and Qt anti-patterns:

    So in 17 unused variables are errors instead of warnings?

    No, its just a bug in the code for a samich.



  • @fcarney said in Recurring C++ and Qt anti-patterns:

    No, its just a bug in the code for a samich.

    Samich... Are you a yinzer?



  • @kent-dorfman said in Recurring C++ and Qt anti-patterns:

    Are you a yinzer?

    Had to look it up. Based on what I read, no. Not sure where I heard sandwich being called samich though. I am in western USA.


  • Moderators

    Being an a-hole as a recruiter:

    What does o() mean?
    What does o.o mean?
    What does o->o mean?
    What does o-->o mean?
    What does o()--<=>--o() mean? Fun fact - crashes MSVC (yes, the compiler, not the compiled program)
    What does [](){;o()++<=>++o();}() mean?


Log in to reply
 

Looks like your connection to Qt Forum was lost, please wait while we try to reconnect.