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

18.04.2017 Sérgio Martins 25 comments

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.