Recurring C++ and Qt anti-patterns

[kshegunov]

Yes, yes, I get that. Then you better start getting used to writing code like this:

int main(int argc, char *argv[])
{
    QApplication app(argc, argv);
    (void) app; // or Q_UNUSED(app);

    return QApplication::exec();
}

Because an unused variable is actually a warning ... and yes in the general case the compiler can't strip it directly, because constructors and/or destructors may have side effects (if they're out-of-line, which they often are). So on the off chance of that, calling a static method on an object is much more benign is my argument. It's a logical error that you'd be able to debug quite easily, and not propagate it into the program runtime. I get people can and will fall for it from time to time, but again, it's rather benign. I'd rather see more strict warnings for implicit conversions than for this ...

[JonB]

@kshegunov
I wish I hadn't picked the "unused variable" warning example :) Please look at the "parentheses" warning instead to compare against.

calling a static method on an object is much more benign [...] that you'd be able to debug quite easily

Then we shouldn't see too many questions about this from ppl :)

P.S.
I have always written:

QApplication app(argc, argv);
return app.exec();

I had never even noticed QApplication::exec() is static, I presumed it was instance. Although it does no harm, I dislike this even more now...!

[kshegunov]

Here's one a bit more convoluted, by yours truly:

GraphDialog::GraphDialog(QWidget * parent)
    : QDialog(parent), chartsModel(&graphMeta)
{
    // ...
    QObject::connect(ui.chartsView->selectionModel(), &QItemSelectionModel::currentChanged, [this] (const QModelIndex & current) -> void  {
        ui.chartEdit->setChart(current.isValid() ? &graphMeta.charts[current.row()] : nullptr);
    });
    QObject::connect(ui.createChartButton, &QPushButton::clicked, &chartsModel, &RbMeta::ChartsModel::addChart);
    // ...
}

where chartsModel is editable (basically an adapter on top of graphMeta) and my types are POD, graphMeta.charts is QVector .

I'm curious whether someone spots it (and no it's not immediately evident, I still haven't hit it, but it's there).

[kshegunov]

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

I'm curious whether someone spots it (and no it's not immediately evident, I still haven't hit it, but it's there).

Well apparently not. I'll tell for funsies:

&graphMeta.charts[current.row()]

Passing a vector element by address is a recipe for disaster when the vector regrows in RbMeta::ChartsModel::addChart (where chartsModel(&graphMeta) is relevant here, again passing by reference in the initializer).

[kshegunov]

Hello, It's me.
I've been saying it for ages, but well here it goes again: C++'s notorious for a good reason ... it's never too late to shoot yourself in the foot. Here's one nasty piece of code:

Say you delegate the constructor of some QObjects. A well-intentioned endeavour:

RbGraphWidget::RbGraphWidget(const RbGraphId & id, QWidget * parent)
    : RbGraphWidget(new RbGraph(this), parent)
{

The above gloriously segfaults, though. Here's a partial trace:

1   QObject::thread                qobject.h                 132  0x7ffff6d9f538 
2   QObject::QObject               qobject.cpp               915  0x7ffff6daacbd 
3   RbGraph::RbGraph               rbgraph.cpp               76   0x55555559384c 
4   RbGraphWidget::RbGraphWidget   rbgraphwidget.cpp         76   0x55555559a3a6
...

So the moral of the story is: the road to hell is paved with good intentions. Anyway, the problem is that QObject requires the parent to have been initialized at the point of the constructor call. The following is correct (reparenting can be done in the delegate):

RbGraphWidget::RbGraphWidget(const RbGraphId & id, QWidget * parent)
    : RbGraphWidget(new RbGraph(), parent)
{

In the usual use case the problem isn't present:

RbGraphWidget::RbGraphWidget(QWidget * parent)
    : QWidget(parent), source(new RbGraph(this)) //< `this` is fully initialized so we have no problem
{

[Kent-Dorfman]

yeah...referencing "this" in the delegated call is bad juju...I still prefer tools that "allow" me to shoot myself in the foot thought.

Here's one that gets people in trouble when they start parsing network buffers.

uint8_t buffer[] = { 0x45, 0x00, 0x12, 0x34, 0x00, 0x00, 0xf3, 0xbb, 0xff, 0x65};
int i = *(int*)&buffer[2];
// bonus if you understand the potential problem...and I don't mean use of C-style casts as opposed to C++ casts

the correct way is either a pragma pack(1) union of the buffer array and a struct containing the interesting elements, or the following:

uint8_t buffer[] = { 0x45, 0x00, 0x12, 0x34, 0x00, 0x00, 0xf3, 0xbb, 0xff, 0x65};
int i=0;
memcpy(&i,  &buffer[2], sizeof(int));

[kshegunov]

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

the correct way is either a pragma pack(1) union of the buffer array and a struct containing the interesting elements

That has a plethora of problems itself. The standard dictates that reading one field and writing another in a union is undefined behaviour. (although compilers implement it correctly).

or the following:

That's the proper way to do it, in my opinion.

[Kent-Dorfman]

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

That has a plethora of problems itself. The standard dictates that reading one field and writing another in a union is undefined behaviour. (although compilers implement it correctly).

Actually, that's kind of news to me. I'd love to see the spec section that defines it as undefined, as that seems to negate the purpose of unions...so I'd hope that compilers implement it correctly. C? C++? or both?

[kshegunov]

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

so I'd hope that compilers implement it correctly. C? C++?

No I was talking about the C++ standard. C99 and C11 are both adamant that it is well defined and does what you'd expect (which thankfully every cpp compiler I've worked with also does, but the cpp standard doesn't make such a provision).

Actually, that's kind of news to me. I'd love to see the spec section that defines it as undefined, as that seems to negate the purpose of unions...

Here's a decent answer:
https://stackoverflow.com/questions/11373203/accessing-inactive-union-member-and-undefined-behavior

[fcarney]

Repeat: "A Loader won't load a Window."

[starkm42]

https://isocpp.org/wiki/faq/freestore-mgmt#delete-zero this might answer you question.