Skip to content

Nailing 13 signal and slot mistakes with clazy 1.3

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:

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.

Categories: C++ / KDAB Blogs / KDAB on Qt / Qt / Tooling

19 thoughts on “Nailing 13 signal and slot mistakes with clazy 1.3”

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

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

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

    1. Sérgio Martins

      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.

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

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

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

  4. Sérgio Martins

    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.

    1. Sérgio Martins

      Yes, for example: (…) // clazy:exclude=check1,check2
      There’s also file-scope comments where you can disable specific checks or clazy altogether

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

    1. Ah sorry, didn’t realise my above query had gone through. First point still worth making though.

    2. Sérgio Martins

      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.

Leave a Reply

Your email address will not be published. Required fields are marked *

By continuing to use the site, you agree to the use of cookies. More information

The cookie settings on this website are set to "allow cookies" to give you the best browsing experience possible. If you continue to use this website without changing your cookie settings or you click "Accept" below then you are consenting to this.

Close