Nailing 13 signal and slot mistakes with clazy 1.3 Create better Qt code by automatically uncovering easy-to-miss errors
24.01.2018
Today I want to share 13 mistakes regarding signals, slots and connect statements and how to find them at compile time with clazy, our open-source static-analyzer for Qt.
Clazy is a compiler plugin which generates warnings related to Qt. You can read more about it in our previous blog posts:
- How to use static analysis to improve performance
- Uncovering 32 Qt Best Practices
- Clazy 1.2 Released
- Clazy Results Visualizer for Qt
Today’s instalment is exclusively about signal/slots, since that’s what the newest 1.3 release focused on.
So let’s start the walkthrough! For completeness a few older checks are also included, as they belong to this topic.
1. connect-non-signal
This check warns when connecting a non-signal to something. For example connecting two slots together:
connect(obj, &MyObj::mySlot, &MyObj::mySlot2); warning: MyObj::mySlot is not a signal [-Wclazy-connect-non-signal]
The above doesn’t make sense, as connect statements should reference at least 1 signal. However, it compiles just fine, since it’s valid C++. With clazy your compiler will get Qt semantics and complain about this mistake.
2. lambda-in-connect
Warns when a lambda inside a connect() captures local variables by reference.
Example:
void myFunction()
{
int localVar = ...;
// program will crash when signal is emitted!
connect(m_object, &MyObj::mySignal, [&localVar] { ... });
}
This usually results in a crash as the lambda might get called after the captured variable went out of scope.
3. connect-3arg-lambda
Warns when using the 3 argument connect() that takes a lambda. The recommendation is to use the overload with 4 arguments, taking a context object to control the connection’s lifetime.
It’s very common to use lambdas to connect signals to slots with different number of arguments:
connect(m_widget, &MyWidget::textChanged, [this] (const QString &text) {
m_receiver->update(text, false);
});
But the above code will cause a crash if the signal is emitted after m_receiver is deleted. To avoid this, just pass a context object as 3rd argument:
connect(m_widget, &MyWidget::textChanged, m_receiver,
[this] (const QString &text) { (....) });
4. lambda-unique-connection
Finds usages of Qt::UniqueConnection where the receiver is a functor, lambda or global function. As stated by the Qt documentation, this connect() overload does not support Qt::UniqueConnection.
5. thread-with-slots
Warns for any slot present in QThread derived classes. Although not necessarily a bug, it’s usually a code smell, as these slots will run in the thread where the QThread QObject lives and not in the thread itself. The best practice is usually to use worker objects, as illustrated here.
6. old-style-connect
Finds connect() statements still using the old SIGNAL()/SLOT() syntax. The Qt 5 pointer-to-member syntax allows you to catch errors at compile-time rather than runtime. A fixit is included for automatically rewriting your connects to the new form.
7. connect-not-normalized
Warns when the content of SIGNAL(), SLOT(), Q_ARG() and Q_RETURN_ARG() is not normalized. Using normalized signatures prevents unneeded memory allocations.
Example:
// warning: Signature is not normalized. Use void mySlot(int) instead of void mySlot(const int) [-Wclazy-connect-not-normalized]
o.connect(&o, SIGNAL(mySignal(int, int)),
&o, SLOT(void mySlot(const int)));
See QMetaObject::normalizedSignature() for more information.
8. overridden-signal
Warns when overriding signals in derived classes. APIs with overridden signals are hard to use, unexpected and bug-prone. To make it worse, Qt even allows you to override a signal with a non-signal, and vice-versa.
To avoid complaining about some benign-cases, clazy won’t warn when both signals have different signatures.
9. virtual-signal
This check warns when a signal is virtual.
Virtual signals make it very hard to read connect statements since they are unexpected.
moc also discourages their use by printing a non-fatal message at runtime:
Warning: Signals cannot be declared virtual
10. const-signal-or-slot
Warns when a signal or non-void slot is const.
This aims to prevent unintentionally marking a getter as slot, or connecting to the wrong method. For signals, it’s just pointless to mark them as const.
Warns for the following cases:
- non-void const method marked as slot
- const method marked as signal
- connecting to a method which isn’t marked as slot, is const and returns non-void
For exposing methods to QML prefer either Q_PROPERTY or Q_INVOKABLE.
11. incorrect-emit
For readability purposes you should always use emit (or Q_EMIT) when calling a signal. Conversely, you should not use those macros when calling something other than a signal.
Clazy will warn if you forget to use emit (or Q_EMIT) or if you use them when you shouldn’t.
12. connect-by-name
Warns when auto-connection slots are used. They’re also known as “connect by name”, an old and unpopular feature which shouldn’t be used anymore.
These types of connections are very brittle, as a simple object rename would break your code. In Qt 5 the pointer-to-member-function connect syntax is prefered as it catches errors at compile-time.
This check simply warns for any slot named like on_*_*, because even if you’re not using .ui files this naming is misleading and not good for readability, as the reader would think you’re using auto-connection.
13. qproperty-without-notify
Warns when a non-CONSTANT Q_PROPERTY is missing a NOTIFY signal.
Although this best practice has been popularized by QML, it’s actually useful for any Q_PROPERTY, as any consumer of properties can make use of the notify, for example Gammaray.
Conclusion
Qt, or more specifically, moc, is very permissive in what it allows, either due to technical limitations, or simply the requirement to maintain source compatibility. I’m of the opinion we should be more strict and only use a sub-set of what it allows, by following these guidelines and enforcing them with clazy, our open-source plugin for clang. Some issues reported by clazy won’t imply a bug in your code, but you should strive to have 0 warnings nevertheless. You’ll sleep much better at night, specially on big codebases.
6. old-style-connect
For that warning it would be nice to switch it off , if no fixit is available. Sometimes we have classes are not in a class hierarchy, but have the same slot. Then you can only use the old-style connect, but we get warnings everywhere.
Hi Gunnar, can you share an example of what you mean ?
class A :public QObject{
slots:
OnEvent();
}
class B : public QObject{
slots:
OnEvent();
}
class C :public QObject
{
C( QObject * client)
{
connect(this,SIGNAL(event),client,SLOT(OnEvent));
}
signals:
event();
}
As client can be passed A or B or any QObject derived class having a OnEvent slot.
Yes, indeed, will fix this
Two remarks:
1. About virtual signals: how else do you do when you have for instance two classes that provide a behaviour (for instance let’s say
struct IFoo {
~IFoo();
virtual void fooChanged(int);
int getFoo() const;
void setFoo(int); // complicated logic that you don’t want to duplicate here
private:
int m_foo{};
}
same for Bar
and a dozen of QObject classes, some that may inherit from IFoo, some other from IBar, some from both, etc:
struct MyObject : QObject, IFoo, IBar {
Q_OBJECT
public: …
signals:
void fooChanged(int) override;
void barChanged(QString) override;
};
sure, a possibility is to instead have Foo and Bar be “normal” QObjects and store them by composition like
struct MyObject : QObject {
Q_OBJECT
public:
Foo foo;
Bar bar;
};
but this really drives allocations up due to the QObjectPrivate & so proliferation, and in practice ends up being quite a bad idea (and let’s not talk of the need to write .foo, .bar everytime).
So what is a correct solution here ? How can Qt make simpler the authoring of common properties shared amongst a set of object ?
2. Do you intend to support Verdigris ? (https://github.com/woboq/verdigris)
Your example looks legit and I’ll silence it in clazy, thanks.
About verdigris, I believe it’s clang based too, so any added stricktness could be implemented there.
No, it’s a header only library which provides alternative macros to Q_OBJECT, Q_SIGNALS & friends (the previous iteration of ~ogoffart’s work did use a clang-based plugin named moc-ng IIRC). I guess I’ll go dig in the source code to see what exactly clazy is using to detect signals & such and if it’s possible to add relevant attributes in verdigris to allow clazy to identify them correctly.
Number 10 is wrong for signals. Having signals non-const mean that no const method can emit a signal. I would rather argue that all signals should be const. For slots it depends on the situation as there are valid cases for slots to be const, e.g. if slot is merely checking state before emitting another signal. Also const methods emitting signals is also useful, e.g. if a method reports that the object has been accessed via signal. Neither changes the state of the objects in question.
I have to agree about the signals and const. The signals themselves do not change state, and there are situations where you want to send signals from methods that do not change state either, so in that case the signal must be const. I just stumbled over one of these cases in Qt Creator code.
Depends on which camp you’re on, many people argue that a slot should do something to the object’s state, otherwise connect to a free function, there are no const slots (and conditional emission can be done via lambda).
That said, I don’t mind relaxing it to allow const signals, as they don’t cause bugs. The goal is mostly to catch a kind of bug which I’ve seen, which is connecting to getters by mistake.
Has clazy 1.3 been officialy released? I didn’t find any announcement on this …
Which clazy version will be included in Qt creator 4.6 (since this issue here seems to be fixed: https://bugreports.qt.io/browse/QTCREATORBUG-15157) ?
The announcements was made in the KDE announce-apps mailling list: https://www.mail-archive.com/kde-announce-apps@kde.org/msg00594.html
For QtCreator it will probably some master revision after 1.3
Is there a magic comment format to disable warnings on particular lines?
Yes, for example: (…) // clazy:exclude=check1,check2
There’s also file-scope comments where you can disable specific checks or clazy altogether
connect-3arg-lambda — I’m not sure this is good advice, at least not as presented here. AIUI, a lambda connect is a Qt::DirectConnection, whereas a connect with a receiver is, by default, a Qt::AutoConnection meaning that simply adding a receiver potentially changes the connection type.
While I’m here, is there any way to disable particular warnings for sections of code? I’ve got a lot of noise from third party code that it would be nice to avoid.
Ah sorry, didn’t realise my above query had gone through. First point still worth making though.
Let’s say clazy’s advice to not use connect-3arg-lambda is correct, what’s incomplete is the suggested fix. I’ll update the readme for this check.
Btw, a few Qt maintainers are in favor of deprecating that connect overload, to force users to think about lifetime and thread affinity/connection types, since we’ve seen bugs in both categories.
Makes sense.
Could you guys also update the Windows binary? I only found 1.2 at http://download.kde.org/stable/clazy.