Recurring C++ and Qt anti-patterns

JonB

@kshegunov
But that is not my point/question. Which is: this piece of code is not the first (or the last) where someone has mistakenly written this. If C++ wants it this way, would it not be a good idea by now for compilers to offer a warning option? There is reason that e.g. C# does not allow it.

fcarney

"Within C++, there is a much smaller and cleaner language struggling to get out."
...
"And no, that smaller and cleaner language is not Java or C#."
Bjarne Stroustrup

kshegunov

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

If C++ wants it this way, would it not be a good idea by now for compilers to offer a warning option?

If this were a potential error, probably. Since this is almost always safe there's no reason to offer a warning.

There is reason that e.g. C# does not allow it.

Which is what exactly?

jsulm

@kshegunov I think the point from @JonB is that people do call static methods on an object by mistake and then wander why the object is not changed (I sometimes see this here in the forums). The compiler could generate a warning, but I doubt people would care enough about those if they do not even notice what they do wrongly :-)

J.Hilk

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

but I doubt people would care enough about those

They don't I have taken over projects that hat on first compile 20k + warnings...

"Every time you compile with warnings, a fairy dies! So don't forget to clap your hands during compile time. Once for each fairy!"

kshegunov

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

@kshegunov I think the point from @JonB is that people do call static methods on an object by mistake and then wander why the object is not changed (I sometimes see this here in the forums).

Yes, I acknowledged that, but it's not an error, nor does it warrant a warning in my mind. One just have to know what they're doing/expecting of said method, which is good approach in every case. ;)

The compiler could generate a warning, but I doubt people would care enough about those if they do not even notice what they do wrongly :-)

That's why I compile with warnings-are-errors before I even consider deploying. The other option is just abysmal ... and can be dangerous depending on which field you're working in. So to everyone out there that ignores warnings: fix your freaking code!

JonB

@kshegunov
I'll try to keep my remarks brief, as I don't want to dominate this thread.

As @jsulm said, my point is that being allowed to call a static method on an instance is not wrong or an error, but it may indicate a programmer mistake. I observe this empirically from the number of cases I have seen, such as the one I quoted from this forum.

If I write a statement like word;, then gcc gives me a -Wunused-value warning. If I write if (word = value) I get a -Wparentheses warning. Neither of these is "wrong", the second one in particular is perfectly useful, yet someone recognised they may indicate commonly made faux-pas. Which the user may ignore, or suppress, at their peril. Personally, I would have liked to have seen a -Wstatic-call-on-instance :)

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.