Skip to content

Uncovering 32 Qt best practices at compile time with clazy Generating compile-time warnings and automatic refactoring for Qt best practices

In a previous blog post we introduced clazy, a clang plugin which makes the compiler understand Qt semantics, allowing you to get compile-time warnings about Qt best practices ranging from unneeded memory allocations to misuse of API, including fix-its for automatic refactoring.

Today we’ll do a round-up and present the checks related to connect statements, Qt containers (including QString) and misc Qt facilities.

Connects

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

2. connect-non-signal

Warns when connecting a non-signal to something.

Example:

 connect(obj, &MyObj::mySlot, &MyObj::mySlot2); 

The above doesn’t make sense, but it compiles just fine, since it’s valid C++.

3. lambda-in-connect

Warns when a lambda inside a connect() captures local variables by reference.

Example:

int a;
connect(obj, &MyObj::mySignal, [&a] { ... });

This usually results in a crash since the lambda might get called after the captured variable went out of scope.

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

Containers

5. container-anti-pattern

Finds temporary containers being created needlessly due to careless API misuse. For example, it’s common for developers to assume QHash::values() and QHash::keys() are free and abuse them, failing to realize their implementation internally actually iterates the whole container, allocates memory, and fills a new QList.

The good news is, fixing these inefficiencies is fun and usually all you have to do is delete code or call another function instead.

Example:

    for (auto value : myMap.values()) // Bad
    for (auto value : myMap) // Good, no temporary allocations

    // Also no allocations, but iterating keys now
    for (auto it = map.keyBegin(), end = map.keyEnd(); it != end; ++it)

    int n = set.toList()[0]; // Bad
    int n = set.constFirst(); // Good

    if (hash.keys().contains(key)) // Bad
    if (hash.contains(key)) // Good

    int sz = hash.values().size(); // Bad
    int sz = hash.size(); // Good

    hash.values().contains(v); // Bad, use std::find() instead

    mySets.intersect(other).isEmpty() // Bad
    !mySets.intersects(other) // Good

6. mutable-container-key

Looks for QMap or QHash having key types that can be modified by external factors. The key’s value should never change, as it’s needed for sorting or hashing, but with some types, such as non-owning smart pointers it might happen. The key types that this check warns for are: QPointer, QWeakPointer, weak_ptr and QPersistentModelIndex.

7. temporary-iterator

Detects when using iterators from temporary containers in for loops.

Example:


// temporary list returned by function
QList<int> getList()
{
    QList<int> list;
    ... add some items to list ...
    return list;
}

for (auto it = getList().begin(); it != getList().end(); ++it)
{
    ...
}

This will crash because the end iterator was returned from a different container object than the begin iterator.

8. detaching-temporary

Warns when calling non-const member functions on temporary containers.


QList<SomeType> MyClass::getList()
{
   // Qt uses implicit sharing, so the list's element aren't actually copied, unless...
    return m_list; 
}

void bad()
{
    // Ouch, triggers deep-copy of all elements, use constFirst() instead.
     SomeType t = myClass.getList().first();
}

In the example above, the container detaches, causing a needless deep-copy of all elements.

9. reserve-candidates

Suggests usage of reserve() calls. Whenever you know how many elements a container will hold, you should reserve space in order to avoid repeated memory allocations. One big allocation before appending items is much faster than multiple small ones and reduces memory fragmentation.

10. qdeleteall

Warns where a qDeleteAll() has a redundant values() or keys() call.

    // Bad, implies a malloc() call and filling a temporary container
    qDeleteAll(bananas.values());

    // Good
    qDeleteAll(bananas);

    // Bad, implies a malloc() call and filling a temporary container
    qDeleteAll(oranges.keys());

    // Good
    qDeleteAll(oranges.keyBegin(), oranges.keyBegin());

values() and keys() create a temporary QList and allocate memory. They are usually not needed.

11. inefficient-qlist; 12. inefficient-qlist-soft

Catches usages of QList<T>where sizeof(T) > sizeof(void*). Use QVector<T> for these cases and read qlist-considered-harmful from Marc’s blog if you’re still using QList.

The soft version will only warn for local lists that are not returned or passed to any function. Otherwise you would get many warnings that couldn’t be fixed due to source and ABI compatibility constraints.

13. range-loop; 14. foreach

Detects usage of range-loop with non-const Qt containers (potential detach/deep-copy). Also warns when you should pass by const-ref in case the type is big or has non-trivial constructors/destructors.

    // Potential deep-copy
    for (auto item : m_items)

    // Good
    for (auto item : qAsConst(m_items))

    // Bad, should probably by passed by <em>const-ref</em>
    for (auto v : getQVariants())
 

There’s also a discontinued check called foreach, it’s disabled by default as range-loop is encouraged.

Misc performance

15. qdatetime-utc

Finds code that should be using QDateTime::currentDateTimeUTC() to avoid expensive timezone code paths.

    // Bad, QDateTime::currentDateTimeUtc().toTime_t() is faster
    QDateTime::currentDateTime().toTime_t()

    // Bad, QDateTime::currentDateTimeUtc() is faster
    QDateTime::currentDateTime().toUTC()

16. qfileinfo-exists

Finds places using QFileInfo(filename).exists() instead of the faster form QFileInfo::exists(filename).

17. qgetenv

Warns on inefficient usages of qgetenv() which usually allocate memory. Prefer using qEnvironmentVariableIsSet(), qEnvironmentVariableIsEmpty() and qEnvironmentVariableIntValue() instead.

18. function-args-by-ref

Warns when you should be passing parameters by const-ref in order to avoid copying big types or to avoid calling non-trivial constructors/destructors.

Misc Qt

19. qt-macros

Finds misuse of some Qt macros. The two supported cases are:

  • Using Q_OS_WINDOWS instead of Q_OS_WIN (The former doesn’t exist).
  • Testing a Q_OS_XXX macro before including qglobal.h.

20. post-event

Finds places where an event is not correctly passed to QCoreApplication::postEvent(). QCoreApplication::postEvent() expects an heap allocated event, not a stack allocated one, otherwise it might crash.

21. missing-qobject-macro

Unless you have a reason not to use Q_OBJECT (like compilation time) you should use it.

Here’s a non-exhaustive list of features depending on this macro:

  • Signals and slots
  • QObject::inherits
  • qobject_cast
  • metaObject()->className()
  • Custom widget as selectors in Qt stylesheets

22. base-class-event

Catches return false; inside QObject::event() and QObject::eventFilter() re-implementations. Instead, the base class method should be returned, so the event is correctly handled.

23. child-event-qobject-cast

Finds places where qobject_cast(event->child()) is being used inside QObject::childEvent() or equivalent (QObject::event() and QObject::eventFilter()).

qobject_cast can fail because the child might not be totally constructed yet.

24. qenums

Suggests using Q_ENUM instead of Q_ENUMS.

25. non-pod-globalstatic

Suggests usage of Q_GLOBAL_STATIC whenever you have non-POD global static variables.

26. wrong-qglobalstatic

Finds Q_GLOBAL_STATICbeing used with trivial types. This is unnecessary and creates code bloat.

QString

27. qlatin1string-non-ascii

Catches non-ascii literals being passed to QLatin1String. It isn’t usually what you want, since your source files are probably in UTF-8.

28. qstring-left

Suggests using QString::at(0) instead of QString::left(1). The later form is more expensive, as it deep-copies the string. Just be sure the string isn’t empty, otherwise at(0) asserts.

29. qstring-arg

Detects chained QString::arg() calls which should be replaced by the multi-arg overload to save memory allocations.

    QString("%1 %2").arg(a).arg(b); // Bad
    QString("%1 %2").arg(a, b); // one less temporary heap allocation

Or you could concatenate with QStringBuilder, if you prefer that style.

30. qstring-insensitive-allocation

Finds unneeded memory allocations such as str.toLower().contains("foo") which should be rewritten as str.contains("foo", Qt::CaseInsensitive) to avoid the temporary QStringcaused by toLower.

Matches any of the following cases: str.{toLower, toUpper}().{contains, compare, startsWith, endsWith}()

31. qstring-ref

Suggests usage of QString::fooRef() instead of QString::foo(), to avoid temporary heap allocations.

Example:

    str.mid(5).toInt(ok) // Bad
    str.midRef(5).toInt(ok) // Good

Where midcan be any of: mid, left, right, and toInt can be any of: compare, contains, count, startsWith, endsWith, indexOf, isEmpty, isNull, lastIndexOf, length, size, to*, trimmed

32. auto-unexpected-qstringbuilder

Warns when auto is deduced to be QStringBuilder instead of QString, introducing crashes.

Example:

   
    // Oops, path is not what you expect it to be
    const auto path = "hello " +  QString::fromLatin1("world");
    qDebug() << path; // Crash if you're using QStringBuilder

Conclusion

That’s all, thanks for reading! I promise the next blog post will be much shorter. I plan to blog every 5 new checks. If you have any questions about building or using clazy, just check the README or ask me directly on IRC (#kde-clazy on freenode).

Please comment with suggestions for new clazy checks, I’ll implement the 5 I like the most for version 1.2.

FacebookTwitterLinkedInEmail

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

25 thoughts on “Uncovering 32 Qt best practices at compile time with clazy”

  1. Thanks Sérgio – this is great stuff.

    I used to have it working with Qt Creator on macOS, but now I just get “error: unable to load plugin ‘ClangLazy.dylib'” errors even though the path to that lib is in LD_LIBRARY_PATH and DYLD_LIBRARY_PATH. It also doesn’t compile with the latest homebrew llvm (which is 4.0), so you need to install 3.9 explicitly.

    Any chance clazy will be added as a tool in Qt Creator at some point? Would be more convenient!

      1. “make sure QtCreator is not using Apple-clang,”

        This was it! I have clazy set up as a compiler in the prefs and it points to the script, but I needed to add an environment variable CLANGXX “//opt/llvm@3.9/bin/clang++” in Creator.

        It would be nice if there were a way to have this set by default in the clazy script based on the llvm install it was built against. The way it is now, CLANGXX has to be set for every project individually. Or maybe use CLAZY_CLANGXX that can be set in the global environment and then checked in the script with a fallback to CLANGXX.

        Voted! Thanks Sérgio!

  2. That should not crash… Static assert is needed.

    const auto path = "hello " +  QString::fromLatin1("world");
    qDebug() << path; // Crash if you're using QStringBuilder
    
  3. qDeleteAll(oranges.keyBegin(), oranges.keyBegin());

    Should the second argument be `oranges.keyEnd()`?

    This is great stuff! I can’t wait to give Clazy a try.

  4. Hey Sérgio,

    very nice job! I’m wondering, would you write a short intro about your tools on the Clang external examples page? And some checkers (like temporary iterator) might be interesting for wilder audience. Do you have plan to move that into Clang-tidy?

    Regards,
    Laszlo

    1. Sérgio Martins

      Probably not moving into clang-tidy (too big effort), but finding some way for clang-tidy to use clazy’s checks seems like a good idea.

  5. We are still in the middle of our qt4-qt5 port, right now we support both from one code base. That means at least some of the checks do not apply (we cannot use the new style connects for example). Is it easy to disable those checks?

  6. Thanks for the very informative article!

    Trying to get the prebuilt binaries to work, I get:
    msvcprt.lib(MSVCP140.dll) : fatal error LNK1112: module machine type ‘X86’ conflicts with target machine type ‘x64’
    Qt5Gui.lib(Qt5Gui.dll) : fatal error LNK1112: module machine type ‘X86’ conflicts with target machine type ‘x64’

    I assume that this means that the prebuilt msvc2015 binaries are requiring that the project is built with a 64 bit version of Qt?

    If so, does anybody have 32 bit binaries for clazy on windows available?

      1. Where should I pass /MACHINE:X86?

        According to the instructions, I just use the following calls for building:

        set PATH=C:\Qt\clazy_v11\bin;%PATH%
        qmake my_proj_file.pro spec=win32-msvc2015 QMAKE_CXX=”clazy-cl.bat”
        nmake

        1. Sérgio Martins

          I’ve just checked, and this works:

          Open clazy-cl.bat, and shove a -m32 somewhere.
          Should look like this:
          @echo off
          %~dp0\clang\clang.exe –driver-mode=cl -m32 -Qunused-arguments -Xclang -load -Xclang ClangLazy.dll -Xclang -add-plugin -Xclang clang-lazy -Wno-microsoft-enum-value %*

  7. Re lambda-in-connect: This is valid when `obj` is a local object and doesn’t outlive the locals captured by reference.

  8. Thanks, it’s great tool!
    The only thing is that I can’t supress warnings from 3rd party code (I use QuickFlux in my project).
    I added
    QMAKE_CXXFLAGS += -isystem $$PWD/../quickflux
    (it’s relative path to the QuickFlux sources) to the .pro file to include it with -isystem instead of -I, but it doesn’t help, there are still warning from it.

  9. Hi, I have Fedora 27 x86_64 with clang-5.0.0 and llvm4.0-4.0.1 provided by Fedora.
    cmake for clazy runs cleanly, but make fails on the first file with errors like the ones below.
    Is clazy supposed to build with clang 5 and llvm 4?
    If I will have to build clang and llvm from source, what are the best versions to use with the current clazy git?
    Thanks, William

    [ 1%] Building CXX object CMakeFiles/ClangLazy.dir/src/checks/level0/qcolor-from-literal.cpp.o
    In file included from /usr/include/clang/AST/DeclCXX.h:19:0,
    from /u/clazy/src/Utils.h:31,
    from /u/clazy/src/StringUtils.h:29,
    from /u/clazy/src/checks/level0/qcolor-from-literal.cpp:22:
    /usr/include/clang/AST/ASTContext.h: In member function ‘llvm::ArrayRef clang::ASTContext::getModulesWithMergedDefinition(const clang::NamedDecl*)’:
    /usr/include/clang/AST/ASTContext.h:939:46: error: invalid conversion from ‘const clang::NamedDecl*’ to ‘clang::NamedDecl*’ [-fpermissive]
    auto MergedIt = MergedDefModules.find(Def);

    /usr/include/clang/Frontend/CodeGenOptions.def:32:1: error: ‘DebugCompressionType’ in namespace ‘llvm’ does not name a type
    ENUM_CODEGENOPT(CompressDebugSections, llvm::DebugCompressionType, 2,

    1. Sérgio Martins

      Hi William,

      clazy needs at least clang/llvm 3.8.
      The problem is that your clang headers don’t match the LLVM ones. Both LLVM and Clang must be the same version. Please double-check you didn’t make any mistake installing the Fedora packages.

      1. Thanks!!! That was the problem. I removed all of the llvm4.0 packages, and then cmake found the llvm 5 packages that matched clang, and clazy built cleanly. I ran the default install into /usr/local, then moved the executables to /usr/bin so they could find Fedora’s clang files. I ran it with
        find . -name ‘*.cpp’ -print0 | xargs -0 clazy-standalone -checks=level2 -p build/compile_commands.json
        and now I have a lot of diagnostics to look at.
        It core dumped on a Qt-generated mocs_compilation.cpp file. Is there a place to file a report?

Leave a Reply

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