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.

How To Edit This Document

Seeing as we have tons of deep links into this document from Gerrit already, in the form "https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews#Includes Item 2.1.1", please don't change section titles or the numbering of existing items¹. If you move something, leave a link in the old spot², if you remove something, change it to strike-through, saying why it was removed and linking to the what replaces it, if any³, if adding, only append to lists, so existing item numbers remain valid.

1. the section numbering shown in the Table of Content is not significant; we deep link to individual sections, so it's the section titles that mustn't change, as they double as hyperlink targets.
2. See e.g. https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews#Polymorphic_Classes Items 3 and 1.5.3
3. See e.g. https://wiki.qt.io/Things_To_Look_Out_For_In_Reviews#Status

Canons

Be aware of the following generally-accepted principles:

1. KDE Binary Compatibilty Page (a must-read, must-understand for anyone doing API reviews)
2. 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)
3. API Design Principles
4. Commit Policy
5. Qt Coding Style
6. Coding Conventions
7. Some books for C++ development:
   1. GoF: Design Patterns (Java and Smalltalk, too, but equally applicable to C++)
   2. Fowler: Refactoring (Java, but equally applicable to C++)
   3. Sutter/Alexandrecu: C++ Standards (condensing all the Meyers Effective X and Sutter Exceptional X books, aged well, better curated than CppCoreGuidelines)
   4. Lakos: Large-Scale C++ Software Design There's a 2nd edition which is multi-part now.

Newly-Added Things

When adding a new Thing, don't forget to provide a rationale. Also see Things_To_Look_Out_For_In_Reviews#How_To_Edit_This_Document.

Gerrit

1. Make sure reviewers are in the attention set. Note that Gerrit resets the attention set when you edit anything in the Reply Dialog. Rationale: Some reviewers only work on attention:self. They won't see your review request if they're not in the attention set.
2. Use topic branches. Rationale: they group changes together across projects and branches, and allow reviewers to review the topic in one session instead of whatever Gerrit believed should be on top of the attention:self display. The relation chain breaks apart on merges and cherry-picks and doesn't work across modules and branches, so it is not a replacement.
3. Push "Done" comments only after you push the new version. Rationale: Avoids pinging the reviewer w/o a new version to look at.
4. Use the following markers in review comments:
   1. (repeats) - there are more instances of this elsewhere (maybe earlier in the patch). It's the commit author's responsibility to find them all, and fix them. There are variations of this: (repeats up/down), [[maybe_repeats]], ...
   2. (pre-existing) - found something that wasn't introduced by the patch. If it's cut-n-paste, suggests that a prequel commit fixing the issue and then rebasing the current patch on top is in order. If not copy-and-paste, suggests to fix in a follow-up commit (or file a Jira ticket to fix). It's the commit author's responsibility to follow-up, by default. But this can be negotiated between commenter and committer. These comments might be created as resolved, in which case they're considered non-blocking for the change.
   3. (nit-pick) - don't use. If you use, explain what you mean by it.

   Rationale: more efficient use of reviewer time

5. Use the following hashtags:
   1. ff - This patch is subject to Feature-Freeze (usually only used when FF deadline nears, to steer reviewers to focus on urgent tasks)
   2. trivial - This patch is trivial. This means only C++ and basic Qt knowledge is required to review it, no domain-specific knowledge. Already split into:
      1. trivial-c++ (C++)
      2. trivial-qml (QML)
      3. trivial-qt (Qt widgets, Core)
      4. trivial-sh (Shell Programming)
      5. trivial-py (Python)
      6. trivial-js (JavaScript)
      7. trivial-java (Java)
      8. (you get the drift) Use prefixhashtag:topic to see them all.
6. As the reviewer, fix commit message typos and other small things directly in Gerrit if that allows you to +2 the change afterwards. Rationale: Saves reviewer's and submitter's work by cutting out one iteration.
   1. As the patch author, be mindful when pushing over already-approved changes (incl. rebasing the chain): An approver may have made edits to your change to approve it.
7. Prefer smaller commits to larger ones. Rationale: Yes, many smaller commits probably take more time to review than one larger one, but the review will be more thorough. As the reviewer, you have more points at where you can stop, as the author, you have more commit messages to provide rationale. And if something goes wrong, reverting a small change is easier than a large one.
   1. Corollary: As a reviewer, feel free to ask to split a patch if you feel it's too large. But don't ask to squash commits unless they really make no sense (or don't even compile) standalone. Rationale: The author of a patch has to sell the change to the reviewers, so if a reviewer thinks a patch could be split, the author should try to do that. OTOH, if the author already has spent the time to chop a change down into smaller patches, then asking her to undo that is not very respectful of her work.

Specifically API Review

1. Use one commit to fix one thing. Rationale: easier to review, and revert, if needed, also easier to backport to older branches if needed/possible. Faster progress as uncontroversial changes can go in while controversial ones are still being discussed.
2. Link the Gerrit change from the comment in the API-Review. Rationale: Better overview, avoids clashes where two people do the same fix.
3. Mark a comment as "Merged", resolving it, when and only when the linked commit's cherry-pick has landed in dev-1.

Commit Message

1. Demand rationale in the commit message. A link to Jira is nice, but no substitute. Rationale: Commit Policy Pt. 8.
   1. Does the commit message answer the questions "Why was the old code wrong/worse?" and "Why is the new code correct/better?"?
2. 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.
3. 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.
4. Check whether a ChangeLog entry should be supplied. If supplied, check it uses the correct tense (often past tense). Rationale: Commit Policy#Change_Log.
5. Avoid passive voice, use imperative mood. Rationale: Consistency with how the vast majority of commit messages are written in Qt.
6. Use correct capitalization, avoid all-lower-case commit messages. Rationale: Easier to read for reviewers.
7. Keep Change-Id footer last, Pick-to: and Task-number: and Fixes: go before, in no particular relative order. Rationale: Consistency.
   1. Exception: Cherry-picks have Reviewed-by and (cherry-picked from) lines below the original Change-Id. That's just how git cherry-pick -x and the cherry-pick bot roll.

API

1. 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
2. Don't default arguments unless they're of Trivial Type. Use out-of-line overloading instead.
   • Rationale: Avoids expensive construction of these objects 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
3. Try to search the other Qt classes API that provide similar functionality, so that we provide easily exchangeable components.
   • Example: QAbstractSlider and QSpinBox have similar interfaces: minimum(), maximum(), value(), valueChanged() signal, etc... Thus, QSlider may be easily exchanged with QSpinBox and vice versa.
4. Try to search the other Qt classes API for the name consistency.
   • Good example: timeout, never timeOut.
   • Bad example: timestamp vs timeStamp - Qt API isn't consistent in this case.
5. Avoid defining symbols in a Qt library that do not reference at least one type from said library.
   • Rationale: Users or other library writers may get the same idea and implement the same function slightly differently, causing UB (ODR violation). The usual suspects for this are Q_DECLARE_METATYPE_EXTERN, Q_DECLARE_TYPEINFO, qHash, relational operators, sin/cos and other math functions, ... These things should all be defined by the library that defines the types involved. If those functions are lacking, and you need them, write a wrapper type or use functions that are named differently.
   • Example: QSharedPointer<char> shouldn't've been exported from QtNetwork (via Q_DECLARE_METATYPE), QtNetwork should have defined its own QNetworkSharedCharBuffer instead. QTBUG-121738, ...

Naming Gotchas

This is a growing list of naming errors non-native speakers may make and what to use instead:

Status (withdrawn)

Withdrawn: https://www.oxfordlearnersdictionaries.com/definition/english/status?q=status Meaning 5

Status is the standing of a person or profession in society. It is not the condition a system is in. Correct Term: State. References: https://www.oxfordlearnersdictionaries.com/definition/english/state_1?q=state Meaning 4 vs. https://www.oxfordlearnersdictionaries.com/definition/english/status?q=status Meaning 2

Iteratable (disputed)

The term Iteratable does not exist.

• Correct Term: Iterable.
• References: https://doc.qt.io/qt-6/qsequentialiterable.html, but see https://codereview.qt-project.org/c/qt/qtbase/+/527219/comment/1de2c8ab_d07aff8f/ and references therein for more opinions.

Mutable (as opposed to const)

While mutable is a C++ keyword, it is not the opposite of const. It means "modifiable even in a const object".

• Correct Term: Mutating, modifiable. non-const, variable (as opposed to constant).

Public Headers

1. Don't move code around when changing public headers. Prefer adding new keywords, attributes, deprecations or removals on a separate line. Don't reorder function just to save on the number of `#if QT_DEPRECATED_SINCE` blocks needed. 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
2. 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

1. 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.
2. 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.
   1. qNN headers are sorted by their eventual name (so not in QtCore group, but in C++ group, and ignoring the qNN prefix). Rationale: when we automatically replace these headers with their std version, the order will then be correct automatically.
      1. Do not include qNNfoo.h and <foo> in the same file. All qNNfoo.h headers include their upstream equivalent (ie. q(NN-3)foo.h or <foo>). Rationale: Prevents duplicate header includes upon automatic renaming (see parent item).
3. 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.
   1. In headers, prefer forward-declaring instead of including. Rationale: Faster compiles, can break circular dependencies.
      1. 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.
4. Don't include qglobal.h anymore. Include the fine-grained headers instead. Rationale: At some point, we want to reap the potential compile-time benefits of not having to include the big qglobal.h anymore, but for that, we need to slowly port away from the monolithic header, not add more users.

Enums

1. 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/
2. 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)
3. New enumerators should be documented as new for version VERSION with \value [since VERSION] See also qdoc's \value command
4. Be clear about what the purpose of the enum is:
   1. An enumeration? Then don't = any values.
   2. A base for QFlags? Then = 0x each value (yes, hex).
   3. A strong typedef? Are relevant arithmetic operators defined?
   4. Anything else? Consider using a class instead. Rationale: gives you more control over the the API.
5. 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.
6. If the enum is scoped, the class is new and the class or one of its subclasses is exposed to QML, add Q_CLASSINFO("RegisterEnumClassesUnscoped", "false"). Rationale: Scoped enums rely on the scope being available to give the enumerators a clear meaning. Without the classinfo entry, the scope is however not enforced on the QML side. Additionally, if two scoped enums would expose an enumerator with the same name, there would be a runtime error in QML without the classinfo entry.
   1. Adding RegisterEnumClassesUscoped to an existing class is a QUIP-6 SiC Type A, so as per QUIP-17 must be accompanied by a [ChangeLog][Potentially Source-Incompatible Changes] tag. Example: https://codereview.qt-project.org/c/qt/qtbase/+/544170
7. When switching over an enum type, prefer to omit a default: case label; list remaining enumerators explicitly. Rationale: switch is a brittle construct: if a new enumerator is added, you can never be certain that you caught all switches over them. This is why -Wswitch is so important: it warns when an enumerator isn't handled by a switch, and -Werror turns that into a hard compile error. But a default: case label prevents the warning (there's also -Wswitch-enum, but that would not be practical for enums with very many values, and would prompt users to revert to inferior solutions like it-else-if chains). To be -Wswitch-clean, code must list all enumerators and not use a default: case label. Exception: While you should keep listing all enumerators for as long as possible, this becomes impractical for large enums and just few handled cases. As a rule of thumb, if default would replace half of the explicit case labels, feel free to use it. And yes, this is asking you to write down up to 100% "useless" case labels, just to enable the warning. It's that important.
   1. Corollary: Avoid checking enum values with if-then-else chains.

Variables

1. Within exported classes, 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: https://lists.qt-project.org/pipermail/development/2024-January/044888.html, QTBUG-121135, and commits linked from there.
2. Make all static or thread_local variables be
   1. constexpr, if possible, otherwise
   2. Q_CONSTINIT const, if possible, otherwise,
   3. Q_CONSTINIT, if possible, otherwise
   4. 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.

3. Braced initializers of variables have the opening { on the same line as the variable definition. Rationale: Consistency; distinguish variable from class definitions.
   1. Prefer writing var = { instead of var{. Rationale: Copy-initialization is safer than direct-initialization. References: https://marcmutz.wordpress.com/2010/08/16/a-case-against-direct-initialisation/, https://marcmutz.wordpress.com/2010/08/24/a-case-against-direct-initialisation-%e2%80%94-the-sequel/
      1. Consequently, initializer_list ctors should be Q_IMPLICIT. Rationale: This is a direct consequence of the parent item.
4. Never use dynamically-sized containers for statically-sized data. Rationale: If you know the size of the container at compile-time, then using a container whose size is determined at runtime (QList, QMap, QVector, QVarLengthArray, std::vector, ...) is overkill. A plain C array or std::array is the better alternative, as they usually operate without memory allocations and can often be marked as constexpr.
   1. For maps, arrays of struct { key, value } in conjunction with std::lower_bound can be used. qt20::is_sorted can often be used to verify at compile-time that the array is sorted. Alternative: Two arrays, one for the keys and one for the values. This can safe executable text size by avoiding padding when key and value are of different size.

Templates

1. Know the difference between "mandates" and "constraints". Rationale: There are two ways in C++ to handle compile-time diagnosable errors in templates: SFINAE and static_assert (or it's little brother, page-long incidental template errors). Use SFINAE (constraints) when the template is or might in the future be overloaded. Mandates (static_assert check) is ok when the template is not and likely never will be overloaded. Example: QList mandates that it's value_type is_nothrow_destructible. Ok, QList is never going to be specialized or "overloaded" for types that throw in the dtor. Example: QObject::connect() mandates (among other things) that signal and slot arguments match. Since connect() is overloaded, it should have constrained instead. The fact that it hasn't makes the many wrappers that we have in Qt (addAction(), GRPC's subscribe(), to name but two) very awkward to write: instead of writing one variadic template that constrains itself on void_t<decltype(QObject::connect(std::declval<Args>()...))>, it has to provide (and manage) several overloads itself. As a consequence, many APIs are still stuck in old-style connect syntax (e.g. QDesktopServices).

Function Templates

1. Don't explicitly specify deducable function template arguments, let the compiler deduce them. In some cases, this is even true for class template arguments (CTAD), e.g. QStringTokenizer. Rationale: Explicit template arguments can break all kinds of things, see e.g. QTBUG-100881.
2. When constraining template functions, use the canonical form: template <typename T, if_condition<T> = true>, don't hide the constraint from QDoc. Rationale: Clumsy, but at least a) safe and b) consistent across Qt (and Qt docs). Example: https://doc.qt.io/qt-6/qstringview.html#QStringView-7 Mechanics: if_X is a template alias for std::enable_if_t<X-expr, bool>. The X-expr can be inline, or, if reusable, be another template alias called is_X or variable template called is_X_v.
   1. The <typename T, typename = std:enable_if_t<X>> often seen elsewhere has the problem that it cannot be overloaded. Just don't use it.

Methods

1. When the method is inline:
   1. Prefer to define it directly in the class body (and skip the `inline` keyword). Rationale: Less keyword bloat, easier to understand the code, cannot make the mistake in the following item.
   2. If defining it directly in the class body doesn't work, e.g. due to circular dependencies, then add the `inline ` keyword on the declaration and never on the definition. Rationale: Prevents certain MinGW errors, cf. e.g. QTBUG-56459.
2. When a getter returns by const-ref:
   1. Make it an lvalue-this overload (const Foo &foo() const & { return m_foo; }) and also provide an rvalue-this overload that returns by value (Foo foo() && { return std::move(m_foo); }). Rationale: This is faster/less code than having to copy from the reference in the caller, and also safer, because the overload set no longer hands out references when called on temporaries.

Properties

1. For new classes that have Q_PROPERTY declarations, the Q_PROPERTY should be marked FINAL unless they are intended to be overridden. Rationale: FINAL properties result in a significant performance boost for QML. See this thread on the devel ML.
2. Existing classes that are exposed to QML, do not add new Q_PROPERTY declarations marked as FINAL, and do not mark existing properties as FINAL. Rationale: Source compatibily breakage, as existing QML code might instantiate the element type and declare such a property.

Timeouts

1. 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
   1. Use QDeadlineTimer if Forever is applicable
      1. Note that QDeadlineTimers cannot be stored. They continue running down the timeout. If you need to store timeouts, store them as chrono types
   2. Otherwise, use std::chrono::{milli,nano}second

New Classes

Documentation

1. Is the documentation for the class complete and in good shape (grammar, style, correctness)?
2. Is the class documented as new with \since?
3. Does the class provide a good overview section?
4. 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!
5. Should the class be mentioned in the What's New in Qt 6 overview documentation?

Value Classes

1. Make sure new (value) classes follow the draft value-class mechanics QUIP-22.
   1. 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.
   2. 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...)
   3. Don't forget Q_DECLARE_SHARED. Rationale: it provides Q_DECLARE_TYPEINFO and ADL swap() to the class.
      1. 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.
         1. In member-swap, swap elements in declaration order. Rationale: Easier to review that all fields are actually swapped, and slightly more efficient to perform memory operations in memory order. References: https://www.kdab.com/four-habit-forming-tips-faster-c/, Item 4
   4. Don't add Q_DECLARE_METATYPE. Rationale: it's automatic now.
   5. 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
      1. Use the usual documentation snippet, don't be inventive: https://doc.qt.io/qt-6/qhostinfo.html#QHostInfo-2 Rationale: see swap() above.
   6. Never export a non-polymorphic class wholesale. Only export public and protected out-of-line members. Rationale: Due to the way MSVC treats inline functions and variables in exported classes, these entities are not really inline, but become part of the ABI. So most of the things mentioned in the BC guidelines about what you can do to inline functions doesn't actually apply. We've had an enormous amount of issues with exported non-polymorphic classes, so the convenience of a single export macro isn't really worth the drawback:
      • https://lists.qt-project.org/pipermail/development/2024-January/044888.html (static constexpr variables in exported classes; alternative solution: don't export the class)
      • https://codereview.qt-project.org/c/qt/qtbase/+/430840/comments/56a6377b_9116ca8e?tab=comments (constexpr in exported classes)
      • https://lists.qt-project.org/pipermail/development/2022-February/042216.html (impossibility to fix C++20 FTBFS in released exported class)
      • qvector_msvc.cpp in Qt 5 (QT_STRICT_ITERATORS vs. non-QT_STRICT_ITERATORS builds because QPolygon : QVector<QPoint> was exported and exported all the QVector template member functions, too, incl. those that were considered inline and therefore not part of the Qt ABI).
      • When we added C++11 features in Qt 4.7 and Qt 5, we thought that making them inline-only would be enough. But in exported classes, they actually became part of the ABI, making C++98 and C++11 builds of Qt binary incompatible with each other. In Qt 6, we now have QT_POST_CXX17_API_IN_EXPORTED_CLASS to prevent the same issue for post-C++17 API. Here, too: the simplest approach is to _not wholly-export_ the class in the first place, leaving these ugly workarounds for those classes that really cannot be unexported (polymorphic ones).
      • tbc...
2. For Q_GADGET classes, see #Polymorphic Classes under "for QObject subclasses".

Polymorphic Classes

1. 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
   1. For subclasses, mark the dtor 'override'. Rationale: Some compilers may warn about inconsistent overrides. Also serves as a static assertion that the base class is actually fit for inheriting from (= has a virtual dtor).
2. Disable copy and move ctor/assignment operator (Q_DISABLE_COPY_MOVE). Rationale: C.67
3. (moved to Things To Look Out For In Reviews#Specifically_QObject_subclasses Item 3)
4. Verify that overridden virtual functions have a) the same default arguments (if any) and b) the same access specifier as the base class' method. Add a comment if the deviation is intentional. Rationale: The whole point of using virtual methods is so that calls to such methods have the same semantics, independent on whether they're called on a base or derived class pointer. If the access level differs, calls that were invalid will become valid (or vice versa). If default arguments change, calls change meaning.
   1. Try to avoid virtual functions with default arguments (see parent item for why). Use NVI (public non-virtual function calls private/protected virtual function) in such cases.
5. Virtual functions
   1. Should be marked by exactly one of 'virtual', 'override' or 'final'. Rationale:: final implies override, override implies virtual.
      1. If the whole class is 'final', use 'override' on individual methods, not 'final'. Rationale: If the class itself is final, then all its virtuals are implicitly final, too. Saying 'final' on every override will just be repetitive. OTOH, we shouldn't lose the protection given by 'override' against unintended non-overrides, so still use 'override'.
   2. Should be public if users should be able to call them, otherwise private, if reimplementations are not supposed to call base class implementation, otherwise protected (ie. if reimplementations are supposed to call the base class implementations). Rationale: Common C++ pattern.
   3. (moved to 4(b) above)

Specifically QObject subclasses

1. Always put the Q_OBJECT macro in. Rationale: Old literature's suggestion to only put it in when there are signals, slots or Q_PROPERTYs is outdated: The Q_OBJECT macro is also needed for qobject_cast'ing, as well as to provide the correct className() at runtime. For public classes, it's a must because we cannot add the virtual overrides behind Q_OBJECT after the class is released (they may not be called by existing compiled code). In the rare case there's a reason to omit the Q_OBJECT macro, put in a code comment explaining why.
2. Always override QObject::event() even if it's just return Base::event(). Rationale: Provides a life-line if we have to add an event handler. We cannot, in general, add new overrides (e.g. mouseMoveEvent()) to a released class, as the new overrides may not be called by existing compiled code. By having at least event() already overridden, we are free to place new code there.
3. Include all moc files (moc_base.cpp and, if any, base.moc) in the main .cpp file ("includemocs"). Rationale: Better codegen and additional warnings like -Wunused-private-field when the compiler sees the implementations of all member functions in a single TU. References: There's a script that can be used to convert whole modules in one go: https://codereview.qt-project.org/c/qt/qtbase/+/409553/7//COMMIT_MSG#16
4. Idiomatic order of elements in QObject subclasses is:
   • { (opening brace of class definition)
   • ␣␣␣␣Q_OBJECT (own line, just after opening { of class definition, so implicitly private, indent: 4sp)
   • ␣␣␣␣Q_PROPERTY()'s, if any (with or without a blank line after Q_OBJECT)
   • ␣␣␣␣Q_CLASSINFO()'s, if any
   • public:
   • ␣␣␣␣enums/flags, if any, each with their Q_ENUM/Q_DECLARE_FLAGS/Q_FLAG
   • ␣␣␣␣ctors and other SMFs
   • ␣␣␣␣getters and other non-slots, if any
   • public Q_SLOTS:, if any
   • Q_SIGNALS:, if any
   • event handlers, if any
   • protected interface, if any
   • private stuff

   Rationale: Consistency.

5. Always re-use QObject::d_ptr, or add a code comment why you can't. Mechanics and Rationale: https://wiki.qt.io/D-Pointer#Inheriting_d-pointers_for_optimization
   1. Exception: If your class is not released together with the rest of Qt, can be built against an older Qt version and has to be BC w.r.t. younger Qt versions, then you can't use this optimization. Note that you can't use *any* private Qt API in your code in that case. Example: QtWebEngine.

RAII Classes

1. Observe QUIP-19 (nodiscard policy). TL;DR: make RAII class ctors [[nodiscard]]
2. Disable copying (Q_DISABLE_COPY). Rationale: There's no general notion for what it means to copy a RAII object, so even if you can come up with one for your particular use-case, it will probably not be a natural one and so will require users to read the docs to find out what's going on. In that case, it's better to not provide the functionality, or, if you do, not in the form of copy ctors that are implicitly invoked by the compiler, but as a separate explicit function call.
   1. Make RAII classes movable or add a code comment saying why they shouldn't be (e.g. if the resulting semantics wouldn't be "natural", ie. require a user to read the documentation). Rationale: Immutability can be indicated by having a const RAII object, so we don't lose expressiveness. OTOH, we can more easily pass such objects into and out of functions, or store them in collections.

Tests

1. Prefer to QSKIP test functions that are not pertinent to the configuration at hand (as opposed to #if'ing their declaration and definition out; just #if / #else with the test body in one and QSKIP in the other clause). Rationale: QSKIPs leave a trace in the logs, so we can more easily see why tests were not run, or verify that they are attempted to be run, at least. Also, gathering CI statistics (like FastCheck) is easier if the test classes have the same test functions on every platform.

1. When comparing two QStringLists using QCOMPARE, prefer the QCOMPARE_EQ macro. Rationale: QCOMPARE_EQ prints both string lists if they don't match

Special Member Functions

1. Preferred order (skip those which aren't pertinent) Rationale: Consistency, logical sequence (start of lifetime, copying/moving, end of lifetime).
   1. Default Constructor
   2. (non-SMF constructors)
   3. Copy Constructor
   4. Copy-Assignment Operator (or Move Constructor)
   5. Move Constructor (or Copy-Assignment Operator)
   6. Move-Assignment Operator
   7. Destructor
      1. For polymorphic classes, see also #Polymorphic Classes Item 1.
   8. Member Swap (following a blank line)
   9. non-SMF assignment operators: not standardized; could be together with their respective non-SMF constructors, or with the copy or move-assignment operators (maybe grouped by taking lvalues (copy assignment) and rvalues (move assignment))
2. The names of SMF and member-swap arguments are always "other". Rationale: Consistency.

Constructors

1. Every constructor should be either `Q_IMPLICIT` or `explicit`. Rationale: Documents that `explicit` wasn't merely forgotten about, but consciously chosen. Exception: Default ctors don't need `Q_IMPLICIT`, as they should always be implicit (see next item).
2. Make default constructors implicit. Yes, that means the previously-idiomatic `explicit QLineEdit(QWidget *parent = nullptr)` is considered wrong now. Rationale: Users expect `{}` to mean the default-constructor. When that is explicit, they need to provide the type: `QLineEdit{}`. You may think that it's better to be explicit, and there's merit to that, but in the end it isn't your decision to make. It's the user's decision, so step out of the way and make default ctors implicit. Mechanics: Instead of default arguments, use overloading and ctor delegation: `QLineEdit() : QLineEdit(nullptr) {} explicit QLineEdit(QWidget *parent);`
3. Never implement copy/move construction using copy/move assignment. Rationale: The assignment operators are normal member functions. They assume *this has all of its class invariants established, and they need to dispose of the old state of *this before assigning the new state from other. A constructor takes raw bits and works to establish said class invariants in the first place. It neither can assume that the invariants are already established, nor does it have existing state to dispose of. So even cases where a ctor from X first delegates to the default-ctor and then goes on to call the assignment operator from X are fishy: Chances are that the code would be more efficient if the ctor was driving the assignment operator rather than vice versa.
   1. The opposite is not true: In general, it's a good idea to use the copy/move ctor in the implementation of the copy/move assignment operator. Keyword: copy-and-swap/move-and-swap idioms.

Move Semantics

1. Distinguish between rvalue references (A &&, where A is not a deduced type (e.g. could be a class template argument and you're in one of its member functions)) and universal references (auto &&, or T &&, where T is a deduced type). Rvalue references are lvalues (sic!), so they should be std::move()'d somewhere in the function. Universal references should, however, be std::forward<T>()'d (or, for auto &&x, std::forward<decltype(x)>(x)'ed – I kid you not!) somewhere in the function. Demand an explanation if either of these isn't true.

Namespaces

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

Operators

1. Prefer to add operators as hidden friends of the least-general class that appears in the argument list. Rationale: QTBUG-87973 (wasn't fully implemented for Qt 6.0, alas) Examples:
   1. if the operator is heterogeneous, you have to pick which type is the less general, and define the operator there Example: QStringView is more general than QString (has less dependencies), so QString/QStringView mixed operators should be defined in QString, not QStringView
      1. esp. relational operators: See comparison QUIP
   2. qHash(): as hidden friend of the key class; if not a Qt class: as a constrained free function template in qhashfunctions.h (or, in very rare cases, the header that defines the class) to avoid implicit conversions
   3. arithmetic operators: op@= as member functions, op@ as hidden friends
   4. streaming operators (QDebug/QDataStream/QTextStream): hidden friends in the streamed class; if not a Qt class: as a constrained member function template of the stream class, to avoid implicit conversions
2. Never break the fundamental relation between equality and qHash/std::hash: ∀x,y ∈ Domain(op==(T,T)) : x == y ⇒ hash(x) == hash(y). Rationale: The hash value of equal elements must be equal, otherwise hash tables don't work.
   1. Corollary: Do not use fuzzy FP comparisons in regular relational operators, overload qFuzzyCompare/qFuzzyIsNull instead. Rationale: An (equality) operator that uses fuzzy comparisons to determine equality will break the fundamental property of equality: that it is transitive (∀x,y,z ∈ Domain(op==(T,T)) : x == y ∧ y == z ⇒ x == z). To see why, take any y. Let x be the maximal value different from y such that x == y and let z be the minimal value different from y such that z == y. Now x == y and y == z, but x != y, breaking transitivity.
      1. Corollary: If equality isn't transitive, then the only consistent hash function (one that fulfills the fundamental relation from the grandparent item) is one that returns a constant value for all inputs (constant at least over each connected subdomain of op==, in case there's more than one).

Lambdas

1. 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 an 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 (Immediately Invoked Lambda Expression), lambdas as private slots, ...
   1. 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 by your efforts.
   2. 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/
2. Omit () (the Lambda parameter list) 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

1. Java-style iterators (QT_NO_JAVA_STYLE_ITERATORS)
2. Q_FOREACH (QT_NO_FOREACH)
3. QScopedPointer → std::unique_ptr Rationale: QScopedPointer's cleanup() looks weird these days, more people know std::unique_ptr.
   1. Use a const unique_ptr to emulate scoped pointers (const unique_ptr cannot yield ownership except by destruction).
4. QSharedPointer/QWeakPointer → std::shared_ptr/std::weak_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.
5. QAtomic* → std::atomic
   1. Exception: static-storage-duration QBasicAtomic* Rationale: The Qt class is verified to not cause runtime initialization.
6. QPair → std::pair. Rationale: Since Qt 6.0, QPair is a type alias to std::pair. The difference between the C++ source and what a debugger or linker would show will only confuse users.

std classes That Shouldn't Be Used in Qt Implementation

1. std::mutex Rationale: QMutex uses futexes on Linux and Windows (possibly Mac, too), which are much faster. std::mutex needs to be a pthread_mutex, for compatibility with std::condition_variable.
   1. 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.
      1. Use QtPrivate::mutex/condition_variable when the code needs to work on Integrity.

---

keep the rest alphabetically sorted

---

QByteArray

1. (also applies to QString) Avoid assigning the first part of a concatenation, followed by op+= calls. Start with a default constructed object and append all parts with op+=/append(). Rationale: When you assign a string to QByteArray, one of two things can happen: a) if the RHS is also a QByteArray, then you've just made a shallow copy and the next append() is guaranteed to detach. b) if the RHS is a non-QByteArray, the LHS will have exactly the capacity required to hold the RHS's data, guaranteeing that the next append() will have to reallocate. If you start with a default-constructed QBA, then the next append() will also allocate memory, but it will allocate according to QBA's geometric growth strategy, ie. in general more than required for just that one append(), so the next append may be for free. It should also be easier for the memory subsystem to deal with temporary allocations of always the same sizes.
   1. Alternatively, use QStringBuilder.

QDateTime

1. Avoid currentDateTime() unless you really need local time. Use currentDateTimeUtc(). Rationale: It's two orders of magnitude faster and stable across DST transitions. Apply timezones only for display to users, everything else is in UTC. References: QTBUG-103392. Credit: Heard it first from Milian Wolff.

QDebug / qWarning()

1. Prefer to use the printf-style if at all possible. Rationale: The printf-style version doesn't require the heavy qdebug.h include, and expands to a lot less code compared to iostreams-style version. Warning and debug code (in production libs) has no business blowing up the executable code. Examples: https://codereview.qt-project.org/c/qt/qtbase/+/264712
   1. Prefer %ls / qUtf16Printable() over %s / qUtf8Printable() in qDebug()/qWarning() messages. Rationale: same as parent: code size. Examples: https://codereview.qt-project.org/c/qt/qtbase/+/263431
2. If you have several similar qWarnings(), Extract Method a Q_DECL_COLD_FUNCTION. Rationale: Q_DECL_COLD_FUNCTION puts the code into a special ELF section to be paged into RAM only when needed, reducing the memory footprint of any application using the code. Compilers also automatically mark any paths that lead to a call of a Q_DECL_COLD_FUNCTION as [[unlikely]], so you don't need to supply the attribute manually. Finally, compilers optimize cold functions for size, not speed, further reducing the executable code size. Examples: https://codereview.qt-project.org/c/qt/qtbase/+/263429, https://codereview.qt-project.org/c/qt/qtbase/+/410970
3. Don't use Q_UNLIKELY on conditions leading to qWarning()/qFatal(). Rationale: While you see this quite widely used in QtBase, it's totally unnecessary, because qWarning() etc are marked as Q_DECL_COLD_FUNCTIONS, so as per the previous item, paths leading to such a call are automatically treated as Q_UNLIKELY by the compiler.

QMap / QMultiMap

1. Avoid using QMap unless you really need the implicit sharing it adds on top of std::map. You do not need implicit sharing if the map is never copied. Passing into and returning from a function, and copies that can be replaced with moves don't count. Rationale: Since Qt 6.0 QMap is merely a shared pointer around std::map. Using std::map, which gets instantiated anyway, tends to save 1.7KiB in TEXT size on optimized GCC 11 builds, sometimes much more, compared to QMap use. That's almost 2KiB of executable code size the CPU does not need to chew through at runtime to do the exact same thing. References: Commit messages in https://codereview.qt-project.org/q/topic:q-to-std-map.

std::numeric_limits

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

std::optional

1. 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.
2. 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
3. When wrapping legacy APIs that want to write into a T out parameter, consider creating the T directly in the optional<> and supplying std::optional::value() to the wrapped function instead of constructing a T on the stack and then moving it into the optional. Rationale: Depending on the T, either one or the other is more efficient. If default-constructing a T is more efficient than value-constructing it (e.g. std::array<int, 100'000>, or just int), then the default-construct-and-move version is more efficient. If a T's default-constructor value-constructs, then creating the T directly inside the optional to save the move is more efficient. Mechanics:

   std::optional<T> one() {
       T t;
       if (read(&t))
           return {std::move(t);} // RVO, but T is moved
       return std::nullopt;
   }
   std::optional<T> two() {
       std::optional<T> r(std::in_place); // value-construct a T inside the optional (does not default-construct)
       if (!read(&r.value()))             // read directly into the optional
           r.reset();
       return r; // note: this depends on _N_RVO
   }
   

Deprecations and Removals

1. 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?
2. for QT_REMOVED_SINCE, does the new function documentation contain a "\note In Qt versions prior to 6.x, this function took/was ..."?
3. 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

Specifically QT_*_REMOVED_SINCE / QT_*_INLINE_SINCE

1. Can the REMOVED_SINCE function actually be removed? There usually needs to be a replacement function to provide SC. If the result is SiC, you cannot REMOVED_SINCE, you need to use traditional DEPRECATED_SINCE. Example: You can replace an (int) with a (qint64), but you can't replace an (int*) or (int&) with a (qint64*) or (qint64&), resp. The latter need to be new overloads and the old ones overloaded, cf. https://codereview.qt-project.org/c/qt/qtbase/+/535653
2. Are the QT6_(CALL,DECL,IMPL)_NEW_OVERLOAD(_TAIL) macros used correctly (and exclusively)? Rationale: Consistency, allowing global search and replace to fix things up come Qt 7. Find existing uses to familiarize yourself with them, on top of the comment in qtversionchecks.h. TL;DR: these macros are only needed if new and old functions differ only in return type.
3. If the patch adds a new removed_api.cpp:
   1. Insist on it to be a separate commit. Rationale: Different audience. Domain experts may not know the details of removed_api.cpp. Also, allows separate teams to make progress separately, without having to wait for an unrelated change to provide a removed_api.cpp Reference: https://codereview.qt-project.org/c/qt/qtbase/+/421365
   2. Is the file in a compat/ subfolder? Rationale: Consistency with the rest of Qt.
      1. There should be exactly one removed_api.cpp per module. Rationale: Consistency. But also tiny speedups, because each removed_api.cpp must be compiled separately (no PCH, no unity build). It probably also just plain won't work if you have one QT_*_REMOVED_SINCE macro used by to different TUs.
   3. Is the compat/removed_api.cpp file added to NO_PCH_SOURCES? Rationale: With PCH, the crucial #define at the top of the file will have no effect because the following #includes will all be skipped as having already been included by the PCH module header).
   4. Does it contain the necessary #defines and an initial QT_*_REMOVED_SINCE block with the usual comment about ordering headers? Rationale: Consistency.
4. If the patch uses removed_api.cpp file:
   1. Is the removed API (incl. the header) in the correct QT_*_REMOVED_SINCE block? Rationale: It won't work otherwise.
   2. Is the header #include of the form prescribed by the prevailing code comment? Rationale: Consistency
   3. Is the header #include correctly ordered w.r.t. the other includes (cf. prevailing code comment)? Rationale: Avoiding merge conflicts.
   4. If the patch uses QT_*_INLINE_SINCE:
      1. Does the header include line have a comment // inlined API (or // also inlined API)? Rationale: Consistency.
   5. If the patch adds a new QT_*_REMOVED_SINCE block for a new Qt version to handle:
      1. Does the new block adhere to 3.4. above? Esp. a new copy of the comment?
5. If inline API needs to be removed:
   1. If not exported, no action needed. Just change (in a SC way, cf. Item 1).
   2. If exported, just warp in QT_*_REMOVED_SINCE, don't move implementation into removed_api.cpp. Rationale: Only MSVC/Windows export inline functions, the other platforms don't. Just wrapping and including the header into removed_api.cpp will do the right thing on every platform. Moving the implementation to removed_api.cpp would add a new symbol on those other platforms. Harmless, but never called, so useless.

Conditional Compilation

1. In general, don't extra-indent code in between temporary #ifdefs, e.g. Qt or C++ version checks. Rationale: We'd like to remove these guards in an automate fashion later on, and whatever tool we'll use will not re-indent (clang-format is incompatible with Qt coding standards, and even if i were, we'd want git-blame to pinpoint the original addition of the surviving lines, not the reformatting commit).

C++ Feature Checks

Qt uses SG-6/SD-10/C++20-style feature macro checks, with a few exceptions that are listed as such in qcompilerdetection.h. From Qt 6.4 onward, you should consider <version> being included by qcompilerdetection.h. Compilers that don't support the file in C++17 mode appear to not have the guarded C++ features for the purposed of Qt API.

1. Use __cpp_lib_... macros to guard the inclusion of new headers, don't use __has_include(). Rationale: Compilers tend to report that they have said include file, but then may #error out when you #include it in an earlier C++ version. Example: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79433 (since fixed, but we had another compiler barf on the same issue seven years later: https://codereview.qt-project.org/c/qt/qtbase/+/560619/comment/833be825_1c63d19b/)
2. Don't use a version check if the minimal required version would be equal to the initial one. Rationale: Communicates to the reader that the initial version of the feature is sufficient, without having to look up (or know by heart) the initial version number.
   1. For new code: use just a version check (ie. omit the check for defined()) if the initial version is available in the minimal C++ standard Qt currently requires. Examples:
      • #ifdef __cpp_lib_span: ok, any version of std::span will do
      • #if __cpp_constexpr >= 201907L: ok, C++17 (required by Qt 6) already guarantees __cpp_constexpr, but we need more
      • #if defined(__cpp_constexpr) && __cpp_constexpr >= 201907L: not ok (in new code): We know __cpp_constexpr it defined, because we require C++17

Decided To Keep Here

(make a proper subsection grouping when moving stuff here)