Things To Look Out For In Reviews

This page is a low-entry-bar staging area for potential project-wide guidelines. After each release, we should go over each of these and decide whether we keep them here, or, preferably, fold them into Review Policy, API Design Principles, Commit Policy, Qt Coding Style, Coding Conventions or a new or existing QUIP.

Canons

Be aware of the following generally-accepted principles:

• KDE Binary Compatibilty Page (a must-read, must-understand for anyone doing API reviews)
• CppCoreGuidelines (a lot of stuff of doubtful quality in there these days, but if you get an F.3 comment, that's where to find what that means)
• API Design Principles
• Commit Policy
• Qt Coding Style
• Coding Conventions

Newly-Added Things

When adding a new Thing, don't forget to provide rationale.

Commit Message

• Demand rationale in the commit message. A link to Jira is nice, but no substitute. Rationale: Commit Policy Pt. 8.
  • Does the commit message answer the questions "Why was the old code wrong/worse?" and "Why is the new code correct/better?"?
• Reject drive-by changes that are really unrelated. A drive-by change is changing something in lines the primary change would touch anyway. An unrelated change changes lines that the primary line would not need to modify. Use common sense: The removal of the trailing } is ok as a drive-by change of adjusting the coding style from if (a) { b; } to if (a) b; but the removal of an include that wasn't obsoleted by the primary change is not. Rationale: Commit Policy Pt. 8.
• If there's an "Amends <sha1>", check it provides the full sha1 (Gerrit will then highlight it as link) and check the target of the link. If it's not there, but you suppose it should, ask for it. Check that Amends and Pick-to are consistent.
• Check whether a ChangeLog entry should be supplied. If supplied, check it uses the correct tense (often past tense). Rationale: [Commit Policy#Change_Log].
• Avoid passive voice, use imperative mood. Rationale: Consistency with how the vast majority of commit messages are written in Qt.

API

• Name static factory member functions create() and non-static factory functions createFoo(). Rationale: For static member functions, the required class name will give sufficient context (QThread::create()). For non-static member functions, the variable name may be something completely different (api.createRequest()). References: https://codereview.qt-project.org/c/qt/qtbase/+/529602/comments/f93def88_ced8e9ca
• Don't default arguments unless it's of Trivial Type. Use out-of-line overloading instead. Rationale: Avoids expensive construction of these object at each call site. Even if the overloaded function calls the other with a default-constructed argument (which it shouldn't, it can pass a pointer e.g. that can be nulled to indicate absence to an -Impl function), it's a win, because the code is then once in the library and not copied at each call site. References: QTBUG-98117, https://codereview.qt-project.org/c/qt/qtbase/+/380545

Public Headers

• Don't move code around when changing public headers. Prefer adding new keywords or attributes on a separate line. Rationale: moving code around makes API-change-reviews needlessly hard. Example: Bad: https://codereview.qt-project.org/c/qt/qtmultimedia/+/528314/1/src/multimedia/video/qvideoframe.h Good: https://codereview.qt-project.org/c/qt/qtbase/+/528393/2/src/corelib/kernel/qpointer.h#34
• Are new (w.r.t. an earlier released version) overrides of virtual functions designed for skipping? Rationale: Existing users of the class (esp. subclasses) may not call the new reimplementations. The implementation must be able to handle that and not mess with the class state. If overrides work in pairs (showEvent()/hideEvent()), one may be called while the other is not. This also needs to be handled correctly. References: https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B, https://codereview.qt-project.org/c/qt/qtsvg/+/529227

Includes

• Include Qt headers always as <QtModule/qheader.h>. Rationale: it's probably the fastest way to include the file while maintaining compatibility with non-per-module-include build systems.

• Group includes in descending specificity / ascending generality: first, module headers, then dependenee Qt modules, then QtCore headers, then C++ headers, then C headers, then platform / 3rd-party headers. Separate groups with blank lines. In each group, sort alphabetically. Rationale: Like in CMakeLists's SOURCES, or removed_api.cpp, or tests, ordering code minimizes conflicts between independent changes.

• Include everything the file needs in-size (Lakos), don't rely on transitive includes. Rationale: Avoids breaking the build if unrelated headers drop unneeded includes.
  • In headers, prefer forward-declaring instead of including. Rationale: Faster compiles, can break circular dependencies.
    • Use qcontainerfwd.h or qstringfwd.h to forward-declare containers (incl. strings) or just string types. Rationale: Avoids depending on implementation detail like template default values or which of the q_no_char8_t/q_has_char8_t namespaces is inline.

Enums

• add a trailing comma to end of enumerators if a line break follows. Rationale: prevents the previous line from needing to change when adding more. Example: https://codereview.qt-project.org/c/qt/qtremoteobjects/+/528328/comment/9b8677ff_4ce9831a/
• enums should either be scoped or have an explicit underlying type. Rationale: prevents the enum's underlying_type to change (which would be BiC). References: http://eel.is/c++draft/enum#dcl.enum-7.sentence-3, https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B#The_Do's_and_Don'ts (Point 3)
• New enumerators should be documented as new for version VERSION with \value [since VERSION] See also qdoc's \value command
• Be clear about what the purpose of the enum is:
  • An enumeration? Then don't = any values.
  • A base for QFlags? Then = 0x each value (yes, hex).
  • A strong typedef? Are relevant arithmetic operators defined?
  • Anything else? Consider using a class instead. Rationale: gives you more control over the the API.
• Try to make sure {} (value-initialization, ie. value 0) means "default". Rationale: Since C++11, {} means "default" (default constructor, e.g.). Also, having 0 be the default saves some space as a) a global variable with initial value 0 doesn't need to be stored in the executable (it ends up in BSS, not DATA) and b) it's easier to construct in a register than a non-zero value (xor r,r), saving executable code size when such types are defaulted arguments.

Variables

• Define static constexpr class variables not only in the .h, but also in the .cpp file. Rationale: This is required with MinGW and happens with both GCC and Clang: when a static constexpr variable found in an exported class is used in a context that requires getting its address, the compiler emits a DLL-importing statement. References: QTBUG-121135, and commits linked from there.
• Make all static or thread_local variables be
  • constexpr, if possible, otherwise
  • Q_CONSTINIT const, if possible, otherwise,
  • Q_CONSTINIT, if possible, otherwise
  • add a comment that it's known to cause runtime initialization

  Don't reorder the keywords. Q_CONSTINIT may be an attribute, so must come first. Try to make one of the constexpr/constinit work if it's easy. Consider Q_GLOBAL_STATIC/Q_APPLICATION_STATIC, too. Rationale: Statically initialized variables have less problems with Static Initialization Order Fiasco (but, except constexpr, may still be subject to Static Destruction Order Fiasco), and improve startup performance of any application linking against the library containing the variable.

Timeouts

• Don't use ints or qint64 for timeouts anymore. Rationale: Poor type safety and less expressive. Qt has adopted chrono-first: https://lists.qt-project.org/pipermail/development/2023-January/043563.html
  • Use QDeadlineTimer if Forever is applicable
    • Note that QDeadlineTimers cannot be stored. They continue running down the timeout. If you need to store timeouts, store them as chrono types
  • Otherwise, use std::chrono::{milli,nano}second

New Classes

Documentation

• Is the documentation for the class complete and in good shape (grammar, style, correctness)?
• Is the class documented as new with \since?
• Does the class provide a good overview section?
• Is it properly linked from related API and overview documentation? In particular, check whether similar or related classes have any \ingroup commands and whether it makes sense the class should also have it!
• Should the class be mentioned in the What's New in Qt 6 overview documentation?

Value Classes

• Make sure new (value) classes follow the draft value-class mechanics QUIP-22.
  • Never use QSharedPointer for d-pointers. Rationale: It's twice the size of Q(Explicitly)SharedDataPointer, so BC means we couldn't change to those even if we wanted.
  • Don't use QSharedDataPointer for the d-pointer anymore. Use QExplicitlySharedDataPointer for the d-pointer, with the macros to allow move-special-member-functions to be =default'ed. Rationale: We used to be able to switch between raw pointer, QSDP and QESDP without affecting BC, but the macros now bake the choice of QSDP or QESDP into the ABI, so we need to get things right from the get-go. References: Comparison of (proposed) QIntrusiveSharedPointer to QSDP and QESDP, highlighing the latter two gotchas (const propagation, eager detaching...)
  • Don't forget Q_DECLARE_SHARED. Rationale: it provides Q_DECLARE_TYPEINFO and ADL swap() to the class.
    • Also ensures the class has a member-swap. When adding member-swap(), use the usual documentation snippet, don't be inventive: https://doc.qt.io/qt-6/qstring.html#swap Rationale: We will eventually add qdoc macros for this, and then porting will be easier if not every class author has come up with her own formulation.
  • Don't add Q_DECLARE_METATYPE. Rationale: it's automatic now.
  • Make move special-member functions inline and noexcept. Rationale: For inline: we want the compiler to see that it's just sapping things around. This does not violate encapsulation. For noexcept: C.66
    • Use the usual documentation snippet, don't be inventive: https://doc.qt.io/qt-6/qhostinfo.html#QHostInfo-2 Rationale: see swap() above.

Polymorphic Classes

• Make dtor out-of-line, even if empty (then =default it). Rationale: pins the vtable to a single TU (and prevents -Wweak-vtable warnings). References: https://bugreports.qt.io/browse/QTBUG-45582
• Disable copy and move ctor/assignment operator (Q_DISABLE_COPY_MOVE). Rationale: C.67

RAII Classes

(tbd)

Namespaces

• Namespaces are called QtFoo, not QFoo. Rationale: prior art: QtConcurrent, Qt, QtPrivate, ...

Lambdas

• Always give lambdas a name. Rationale: The primary benefit of the STL is that it gives names to algorithms. As the reader of such code, I can choose to trust the name of algorithm, and continue analyzing, or I suspect that the algorithm (name) is wrong and dig into its implementation. The point is: the name gives me a choice. If you write a raw loop, you don't give users that choice. You force them to understand the whole thing. The same is true for lambdas. I can pass a raw lambda to an algorithm, and thus force the reader of the code to understand the lambda, or I can give the lambda a name and give the reader a choice to trust the implementation of the lambda matches its name and continue, or to distrust it and dig into the lambda implementation. Exceptions: IILE, lambdas as private slots, ...
  • Try not to just use the English prose of the lambda's C++ implementation as a name, but try to find a domain-specific name. Rationale:' Naming is hard. But readers are rewarded for your efforts.
  • Stateful lambdas are not name-able. Don't try. It will be awkward. Use lambda-returning-lambdas to separate the lambda name from the state. Rationale: Always hide the complexity in the definition of the lambda and make the use of it as pleasant as possible. fieldEquals(foo) also makes the lambda reusable: fieldEquals(foo), fieldEquals(bar). Without lambda-returning-lambda, you'd end up with fieldEqualsFoo and fieldEqualsBar. And QtCreator's CTRL-SHIFT-R won't change _those_ names. Example: https://codereview.qt-project.org/c/qt/qtbase/+/529905/comment/7f31e85d_0492ca24/
• Omit () when empty, unless required by a following -> ReturnType, mutable or noexcept. Rationale: The guideline to supply an empty parameter list comes from the time when MSVC required them, circa 2012. We're long past that now, so remove the visual clutter. Upstream C++ is dropping the need for empty parameters in more cases in later revisions of the standard, so this will become more prevalent, not less.

Class-Specific Usage Guidelines

Qt Classes That Shouldn't Be Used in Qt Implementation Anymore

• Java-style iterators (QT_NO_JAVA_STYLE_ITERATORS)
• Q_FOREACH (QT_NO_FOREACH)
• QScopedPointer → std::unique_ptr Rationale: QScopedPointer's cleanup() looks weird these days, more people know std::unique_ptr.
  • Use a const unique_ptr to emulate scoped pointers (const unique_ptr cannot yield ownership except by destruction).
• QSharedPointer/QWeakPointer → std::shared_ptr/std::shared_ptr Rationale: QSharedPointer's implementation is poor and requires twice the number of atomic operations on copy than std::shared_ptr. We want to remove the class in Qt 7.
• QAtomic* → std::atomic
  • Exception: static-storage-duration QBasicAtomic* Rationale: The Qt class is verified to not cause runtime initialization.

std classes That Shouldn't Be Used in Qt Implementation

• std::mutex Rationale: QMutex uses futexes on Linux and Windows (possibly Mac, too), which are must faster. std::mutex needs to be a pthread_mutex, for compatibility with std::condition_variable.
  • Exception: The std::mutex/std::condition_variable combination is more efficient than QMutex/QWaitCondition, because the latter has two mutexes while the former has only one.

std::optional

• Don't use value(), has_value(), use pointer-compatible subset: if (opt), *opt, opt->foo. Rationale: has_value() is just needless verbosity, but value() is downright dangerous, because it's a checked version of op* and throws an exception if !*this.
• Avoid the default constructor, explicitly use std::nullopt. Rationale: GCC has a long-standing bug which causes the default constructor to emit maybe-unused warnings. Example: (incl. error message and links to upstream bug reports) https://codereview.qt-project.org/c/qt/qtlanguageserver/+/413811

std::numeric_limits

• Guard against Windows' min/max defines like this: (std::numeric_limits<int>::max)(). No code comment needed. It's used ubiquitously in Qt.

Deprecations and Removals

• Does the documentation reflect the deprecation, and mention the version it is first deprecated for (see also qdoc's \deprecated command? Does it give the user a rationale, as well as a suggestion for alternatives?
• for QT_REMOVED_SINCE, does the new function documentation contain a "\note In Qt versions prior to 6.x, this function took/was ..."?
• When deprecating a signal or a slot, use QT_MOC_COMPAT to enable warnings also on string-based connect(). Example: https://codereview.qt-project.org/c/qt/qtbase/+/531860

Decided To Keep Here

(make a proper subsection grouping when moving stuff here)