Recurring C++ and Qt anti-patterns

[aha_1980]

When doing code review, you always see things that hurt your eyes, and it's impossible to get away from them.

I thought it would be good to collect them as bad example - Feel free to add your's to the hall of shame.

So here we go, one thing I see again and again:

int items = 42;
QString s = "In your basket are " + QString().setNum(items) + " items.";

// better so:
// QString s = tr("In your basket are %1 items.").arg(items);

And if you think that cannot be topped, here is my second example:

int ch = 42;
QString cmd = "ch";
if (ch < 10) cmd += "0";
cmd += QString().setNum(ch) + ".foo";

// better so:
// QString cmd = QString("ch%1.foo").arg(ch, 2, 10, QChar('0'));

Regards

[J.Hilk]

@aha_1980
😆
Maybe also add how it should be done ?

[aha_1980]

@J.Hilk

Maybe also add how it should be done ?

Done :)

[fcarney]

This one I have found amusing:

QString s = tr("Some text: " + variable + " some more text" );

Or this:

QString s = tr("Some text: ") + variable + tr(" some more text" );

[aha_1980]

@fcarney nice one! that makes translations rather pointless.

[aha_1980]

I just saw the following again:

auto *p = new ...;
// ...
if (p != NULL) {
  delete p;
  p = NULL;
}

which can be shortened to:

delete p;
p = NULL; // p = nullptr in C++11 and upwards

because delete does the check anyway.

[J.Hilk]

@aha_1980
well, I would give the creator here the benefit of the doubt and they that's refactored code and used to be p->deleteLater() :-)

[aha_1980]

Hi @J.Hilk,

Yeah, but in that case it was pure C++ without Qt. Otherwise you would be right.

[fcarney]

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

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

[fcarney]

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.

[aha_1980]

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

[fcarney]

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

[ODБOï]

@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

[aha_1980]

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

[fcarney]

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

[ODБOï]

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

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

[fcarney]

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.

[JonB]

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