Hi, Il 03/09/25 07:18, Thiago Macieira ha scritto:
Re: https://codereview.qt-project.org/c/qt/qtbase/+/672087TL;DR: I've got a solution to make the new-style QObject::connect() match the behaviour of the old-style string-based one, which due to the nature of the virtual table dispatching, would not deliver signals to slots in class sub- objects already destroyed. But it isn't perfect. So I'd like a discussion:
My take:
a) should this functionality be provided at all?
Yes, but AFAICT it's impossible to provide it in a way which is correct and complete (like the string-based used to be), unless we force our hand in some place.
b) should it be provided by default for PMFs? [*]
Yes, but I think we should clarify what PMF actually means here...
c) if it's provided for PMFs, should they be allowed to opt out? d) should it be provided by default for callbacks other than PMFs?
... because if affects the answer here:A) if you're connecting to a non-static member function of a QObject subclass, then the correct thing to do is to disconnect/not to activate the slot if receiver's type is no longer the one declaring that non-static member function.
I have no idea how to achieve this reliably without RTTI: you can connect to an arbitrary PMF of a QObject subclass that doesn't use Q_OBJECT and therefore whose "distance" from QObject is a wrong metric:
struct A : public QObject
{
Q_OBJECT
~A() { emit aSignal(); }
signals:
void aSignal();
};
struct B : public A
{
B() { connect(this, &B::aSignal, this, &B::aSlot); }
void aSlot();
};
~A() will still call the slot, won't it? But it mustn't.
To complicate things further, the type of the receiver isn't the one
that actually matters:
struct A : public QObject
{
Q_OBJECT
~A() { emit aSignal(); }
void aSlot();
signals:
void aSignal();
};
struct B : public A
{
B() { connect(this, &B::aSignal, this, &B::aSlot); }
};
Now A::aSlot() must be called instead.
We could bring back the "rule" that you should only connect to PMFs of
classes that do have Q_OBJECT, or at least enforce it as best practice,
say with some Clazy warnings (which already warns for QObject subclasses
without Q_OBJECT in general). With this rule in place, would we be able
to go back to the behavior of string-based connections?
B) if you're connecting to something else than a PMF, and you're passing a context object, then the check applies to the context. Not passing a context object is _definitely_ bad practice because of lifetime and threading issues; right now there's an opt-in (QT_NO_CONTEXTLESS_CONNECT, also set by QT_ENABLE_STRICT_MODE_UP_TO) but we should consider raising this to a deprecation level.
e) if there's a flag to opt in or out, what do we call the Qt::XxxConnection?
Is there a valid use-case for even wanting an opt-out mechanism? String-based connects don't have an opt-out one.
Longer version: I found a neat solution to implement a check for the dynamic class type that does not involve RTTI, and will restore the behaviour that we used to have with the string-based connections. The solution is to count how many meta object superClass() we have until we reach the QObject's, and store this count in the QObjectPrivate::Connection object. I've also done that without increasing that object's size, because there are members unused when connecting to a QSlotObjectBase.
There are two drawbacks: 1) it adds overhead to the activation of the slots, because it must count meta objects for every Connection prior to activation. As a singly-linked list, this is O(n), and though N is usually very small (99-percentile less than 10), it may cross library boundaries and may thus run into cache misses. The same operation is required on connect() itself, but the overhead there is relatively smaller compared to the rest of the work. I have not measured it and quite frankly I don't know what a good benchmark would be, if the issue is going to be cache misses. But I am wary of just adding content to QObject::doActivate(), which should be FAST.
Yeah, this implementation doesn't sound optimal.If we force our hand elsewhere and require RTTI instead, would we able to provide a faster one, by checking the type_infos of the associated objects? (This would require using non-portable code, surely, but it's not that such a thing should scare us away.)
2) I think it's safe to apply this by default when the receiver is a PMF and the class of the PMF has destroyed (implemented on the commit after the above, 672088), but what about when the receiver is a static or non-member function, or lambda or functor? The lifetime of the receiver's current dynamic class is not tightly bound with the callback. An example of this is Q_APPLICATION_STATIC: QObject::connect(app, &QObject::destroyed, app, reset, Qt::DirectConnection);
Here I think we may need a separate connect() overload, because the 3rd argument is pointless since one is calling a static function.
Why a different overload rather than just using the existing overload of connect()? Because the existing overload has shown that it's terribly error-prone. The hope would be that a connectToStatic() would help avoid such issues.
(Alternatively, the 3-arg connect should distinguish between connecting to function objects in general, and connecting to pointers to (static) functions; disallow the former, and allow the latter.)
The dynamic class of the receiver stopped being QCoreApplication by the time
~QObject emitted destroyed(). Should the signal be delivered to reset()?
Obviously the code above expects it to do so. In fact, that's the expectation
for every use of &QObject::destroyed, because otherwise nothing would ever
run.
But how about
QObject::connect(notifier, &QWinEventNotifier::activated, this,
[this, registerForNotification] {
emit valueChanged();
registerForNotification();
});
QObject::connect(menuAction, &QAction::changed, q, [this] {
if (!tornPopup.isNull())
tornPopup->updateWindowTitle();
});
QObject::connect(reply, &QNetworkReply::errorOccurred, query, [this, reply] {
m_networkErrors << reply->errorString();
});
Those are lambdas that, by capturing [this], are effectively member functions
in disguise, but there's no way we can tell that. Invoking those callbacks
after the members used in bodies of the lambdas have been destroyed is UB.
But that's the reason for passing the 3rd argument, isn't it? To make sure that deleting `this` will disconnect. (At least, that's the idea; at the moment it's not foolproof, thus this thread...)
Even if the callback is a static or non-member function, it could still find the receiver via some global variable. And I don't know if we'd want a different behaviour depending on whether the callback is a lambda or a function pointer.
I'd say that this is _way_ less likely to happen than 99.99% of the trouble we've been having with connects to PMFs(). What people do _in_ the slot is a separate issue than the fact that we should or not should activate them.
Even the distinction between PMFs and others could be a pitfall. Take the first
example from QWinEventNotifier and let's say the original code was
QObject::connect(notifier, &QWinEventNotifier::activated,
this, &QWinRegistryKey::valueChanged);
That code would have had the benefit of the protection. But then after some
bug-fixing or additional functionality, it became the lambda above, which does
not have the protection. That could lead to a subtle new bug[*] that may
escape detection during unit-testing and get found only by users.
[*] Right now, we don't have the protection against calling a PMF in a
destroyed sub-object, but we have an *enforcement* in debug mode, so if the
delivery could have hit the PMF before the change, it should have been seen
and fixed. Since we have the enforcement, I think that if we provide the
functionality, it should be active for PMFs by default.
That's a separate can of worms, and also, once more, why people should always use the 4-arg connect (and avoid connecting to complicated lambdas).
My 2 cents, -- Giuseppe D'Angelo | [email protected] | Senior Software Engineer KDAB (France) S.A.S., a KDAB Group company Tel. France +33 (0)4 90 84 08 53, http://www.kdab.com KDAB - Trusted Software Excellence
smime.p7s
Description: Firma crittografica S/MIME
-- Development mailing list [email protected] https://lists.qt-project.org/listinfo/development
