Things To Look Out For In Reviews

From Qt Wiki
Jump to navigation Jump to search


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.

If jargon or abbreviations here are all Greek to you, consult the Glossary and, if necessary, request an entry in it.

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.
  8. The Qt Glossary

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:trivial 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.
  8. If you give a +1 instead of -1 or +2, consider leaving a comment explaining why it's not +2. Rationale: A +1 without comment is a bit like a -1 without comment: leaves the patch author guessing what's wrong with the patch. Be nice, give an explanation.

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.
    1. Ask for drive-by changes to be spelled out explicitly in the commit message. Rationale: Confirms that reviewer and author are on the same page. Example: https://codereview.qt-project.org/c/qt/qt3d/+/553115
  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, sometimes present tense + "now"). 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.
  8. Specifically cherry-picks:
    1. If you change the commit to make it work in an older branch (other than line-conflict resolution, ie. fixing the <<<</====/>>>>>), add what you did to the commit message of the cherry-pick. Rationale: Alerts reviewers to changes that may be subtle or hidden in a larger change and are not immediately obvious when comparing the cherry-pick with the commit it was picked from. Mechanics: Add "Merge conflict resolution for N.M:\n- first item\n- second item..." near the end of the commit message (N.M is the product's branch you're picking to). Example: https://codereview.qt-project.org/c/qt/tqtc-qtbase/+/579327/5//COMMIT_MSG#20

API

  1. Name static factory member functions create() and non-static factory functions createFoo().
  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:

Keep sections ordered alphabetically

Get

In Qt, unlike Java, the get- prefix on functions is not used for mere getters (cf. https://wiki.qt.io/API_Design_Principles#Naming_Boolean_Getters,_Setters,_and_Properties). If a function is called getX, it implies user interaction (QColorDialog::getColor()). It is also traditionally used for decomposing a complex type into constituents (QColor::getHsv()), with out-parameters. This use should be considered deprecated as we don't want to use out parameters anymore (return a struct instead).

Iteratable (disputed)

The term Iteratable does not exist.

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

Status

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

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
  3. Always protect min/max calls from Windows min/max macros. No code comment needed. Mechanics: add extra parentheses (std::min)(a,b), (std::numeric_limits<T>::min)(). Ditto max(). Rationale: Even though our own TUs try to avoid the issue by including qt_windows.h instead of Windows.h (or whatever the header is), public headers can be included in user projects that don't do this, so play along and don't defend against the macros in a way that alters the meaning of user code (#ifdef #error or hard-#undef min/max), but use the idiomatic evasive work-around. No code comment needed, as it's idiomatic.
    1. Exception: if you need to define a min() or max() function, this trick doesn't work, and #erroring out if min/max are defined is a good compromise.
    2. Apply this pattern also in .cpp files and private headers. Rationale: Unity builds. Also: if you don't do it consistently, how do you know you do it when it matters?

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.
  5. The following headers are guaranteed to be included by qcompilerdetection.h (included by any other Qt header except qNN ones):

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.

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.
      1. If the inline function is defined in a header other than the one in which it was declared, add a comment // defined in other.h. Rationale: Consistency, and a nicety to reviewers.
  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.

Macros

  1. Macros that are to be used in function scope should always use do {} while(false) wrapping, so those naturally then require a user-supplied semicolon. See e.g. QVERIFY. Rationale: Prevents errors as discussed in e.g. https://stackoverflow.com/a/154264.
    1. In other cases, macros expand to something that cannot accept ; at the end; e.g. Q_DECLARE_SHARED.

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 (noexcept) 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
        2. In member-swap, use a member's member-swap (x.swap(other.x)) if available, otherwise qt_ptr_swap(x, other.x), if is_pointer<decltype(x)>, otherwise std::swap(x, other.x). Do not use qSwap() unless you don't know the type you're swapping (e.g. its a template argument). Rationale: Compile-time speed. References: https://lists.qt-project.org/pipermail/development/2023-September/044498.html
    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 swapping 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:
  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.
      1. Use move-and-swap in the move-assignment operator (if any). Rationale: Since RAII classes handle resources other than memory, we must make sure that the LHS reource gets freed, even on a mere x = std::move(y);.

Tests

  1. (For QSKIP see below.)
  2. When comparing two QStringLists using QCOMPARE, prefer the QCOMPARE_EQ macro. Rationale: QCOMPARE_EQ prints both string lists if they don't match

Suppressing tests

See also the documentation on selecting appropriate mechanisms to exclude 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.
  2. Where a test is pertinent but known to sometimes crash, it is better to check against some precondition of the crash, e.g. QVERIFY(ptr) before dereferencing ptr, so as to trigger a normal failure, than to skip the test. Skipping such a test should be used as a last resort when no precondition of its failure can be found. If resorting to QSKIP for such a crash, its message SHOULD contain the ID of a bug-report that will not be closed until the crash is fixed and the QSKIP is removed. Rationale: a crash prevents the test-system from reporting properly, or running any later tests in its suite. It is better to make it show up as a failure (which may be guarded with the mechanisms below) than to hide the problem by skipping it. If skipping, it should still be seen as a bug to which we need to return and for which we need to find a fix.
  3. Where a test is pertinent but (in some clearly-identifiable context) consistently fails, use QEXPECT_FAIL in preference to other mechanisms. The message for this SHOULD contain the ID of a bug-report that will not be closed until the QEXPECT_FAIL is removed. Of course, the QEXPECT_FAIL can be conditioned in all the usual ways to limit it to the context where it fails (but see next item). Rationale: This makes clear that there is a known bug here, says where to look for more details and makes it easier for someone working on that bug-report to find the test-cases they can use to reproduce it. It also makes the issue visible in the bug-tracker so that a contributor looking for something to fix might find it, and relevant teams working on Qt can filter it into their backlogs.
  4. Do not condition QEXPECT_FAIL on the same condition as the check it's guarding; if you cannot determine the precondition of failure in terms of the platform, version, libraries in use or other kindred contextual factors, you've got a flaky test that should instead be blacklisted (see below). Rationale: QEXPECT_FAIL marks a known bug and tells someone investigating the bug how to investigate it. If you don't know the context that triggers the bug, you can't tell them what they need to know.
  5. When a test is unreliable (in some context) – that is, it sometimes passes and sometimes fails, with no clearly-understood rhyme or reason to when it does which – it is a flaky test and should be blacklisted. This is done by adding an entry to a BLACKLIST file, see the introductory comment in qtbase/src/testlib/qtestblacklist.cpp for details; it causes the test outcome to be reported without failure of the test causing the test-run to fail. Each line describing a context in which a specific test's result is to be so ignored SHOULD be marked with a # QTBUG-nnnn comment identifying a bug-report that will not be closed until this blacklisting is removed. Do this even if it means repeating the same comment on many lines. Rationale: blacklisting makes clear that we don't know how to reproduce the issue reliably. Having a bug-report still open makes the issue visible in the bug-tracker so that a contributor looking for something to fix might find it, and relevant teams working on Qt can filter it into their backlogs. Marking each context line makes clear that the comment applies specifically to that context. (Leaving a comment before a block, in contrast, is apt to be ambiguous as it is unclear where the end of that comment's relevance is. Even if it applies to the whole file when it's added, later additions to the file may then appear to be covered by it.) Having the test still run, with its results reported, makes it possible to review test logs and see whether the test does in fact still sometimes pass and sometimes fail.

C++-Feature-Specific Guidelines

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
      1. The move-assignment operator should be generated from one of the QT_MOVE_ASSIGNMENT_OPERATOR_IMPL_VIA_{MOVE_AND_SWAP|PURE_SWAP} macros. Use PURE_SWAP only if the objects hold only memory, no other resources (locks, handles, ...). RAII classes should never use PURE_SWAP. If in doubt, use MOVE_AND_SWAP. Rationale: The difference between PURE_SWAP and MOVE_AND_SWAP lies purely in how long the old state of the moved-to object lives on (both are nothrow, so no difference there). In PURE_SWAP, it lives as long as the rvalue lives on (which may be an xvalue, so an lvalue, so potentially for a long time, because the lvalue's lifetime doesn't need to end at the end of the full-expression). In MOVE_AND_SWAP, it's reliably gone after the op= call. This is what users expect when you assign to a mutex locker, e.g.. For mere memory, preserving the capacity() of the moved-to object in the moved-from object may make filling the latter faster, and PURE_SWAP is also less code (one move ctor and one dtor less, to be precise), so PURE_SWAP is preferred there. Since we can't turn an (inline) PURE_SWAP into a MOVE_AND_SWAP, if in doubt, MOVE_AND_SWAP should be preferred. It can always be relaxed to PURE_SWAP afterwards.
    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.
  3. Copy SMFs of implicitly-shared classes should usually not be noexcept. Rationale: A noexcept does not state the status quo. Since removing it is BiC, it can never be removed once added, so it's a promise for the future. We currently knowingly implement Copy-on-Write incorrectly by ignoring the unsharable state required when handing out references to internal data. It is not inconceivable that the current push towards safety brings proper implicit sharing back at some point, and then a copy SMF needs to be able to deep-copy, so can't be noexcept. Current absence of functions that hand out references to internal state is not a get-out-of-jail-free card for a noexcept ctor, because such functions could be added in the future, rendering the noexcept a lie. References: http://www.gotw.ca/gotw/044.htm (yes, this looks so 90s, but it's Herb Sutter's page, and just goes to show how long this issue has been known).
    1. Exception: When a deep-copy would be noexcept, if that's possible, the copy SMFs can be noexcept.


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.

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. Rationale: Initializing all the enumerators makes a reader of the code wonder why that's done: Are some missing?
      1. Exception: If some enumerators are #ifdefed out, then use initializers to maintain the numeric values of following enumerators across configs and platforms.
    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.

Exceptions

noexcept

  1. Only functions that cannot fail to meet their post-conditions can be noexcept. Failure to meet post-conditions can stem from a variety of reasons, including a) having preconditions that are not met, b) calling a Posix Cancellation Point or another noexcept(false) function (which may prematurely end the function), or c) allocating memory or other resources (file handles, mutexes). Rationale: This is the Lakos Rule (latest version: https://wg21.link/p2861). We should track what the standard does and not invent rules of our own. Specifically for preconditions, there are vague plans to make them testable from QtTest by making Q_ASSERT() throw an exception instead of calling abort() on failure; any noexcept function in-between will cause this to not work. Specifically for (b), some Posix systems implement thread cancellation by throwing an exception-like object to unwind the stack (e.g. glibc-based ones). Such exceptions must not be swallowed by user code (incl. Qt code) lest thread cancellations ends in std::terminate().
    1. Exception: Like the std smart pointers, our smart pointers operator*() may also be noexcept, but then they must not contain a Q_ASSERT(). Rationale: When in Rome^WC++, do as the Romans do^W^Wstd does.
    2. Distinguish Q_ASSERT()s that check for precondition violations (= user is responsible) and those that check internal state/invariants in our code (= we are responsible). The former requires the function containing them to be noexcept(false), the latter doesn't. Rationale: The Lakos Rule is about contract violations, not about internal consistency checks. If internal consistency is found to be in disarray, recovery is likely impossible. If a call contract is violated, OTOH, that may be very much recoverable, because it's detected before "something bad" may happen. We'll hopefully get separate macros for the two, but for now, be aware of the two different use cases of assertions.

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.

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

Constraints Howto

  1. Prefer std::disjunction_v<x,y> over x_v || y_v. Ditto conjunction/&& and negation/!. Rationale: The named constructs implement short-circuit evaluation at the template level. Regular || also does that, but the templates appearing in the unevaluated branch are nevertheless instantiated (the compiler "reaches into them", for the nested ::value). The named constructs avoid that carefully. This is useful for correctness reasons (akin to p && p->data in C/C++), but even if you don't need the correctness aspect, it's still faster than raw binary operators, because template instantiation costs a lot.
  2. Never chain is_same in a disjunction. Take a helper variable template defaulting to false and specialize it to true for all the types you want to match. Rationale: O(1) template instantiations for specializing helper vs. O(n) template instantiations for is_same chains.
    1. Exception: two is_same in a row are probably still ok (but where there are two, a third may appear on short notice ;-)).
    2. Exception: if you really need to prevent users from specializing the helper, is_same chains may be some help, but users can, in general, specialize anything they want (except some std templates, where it's UB), so it's really hard to defend against a determined user.

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. At the time of writing, the if_X in QDoc's \fn needs to be qualified with the scope it's defined in.
    1. The <typename T, typename = std:enable_if_t<X>> often seen elsewhere has the problem that such functions cannot be overloaded even if the conditions X are mutually exclusive for different overloads, because default arguments are not part of the function signatue. Just don't use it.
    2. When constraining a documented function this way, add a \note This function only participates in overload resolution if... followed by a prose description of the constraint. Rationale: There's currently no documentation for these things in their own right, something that we'll hopefully fix once we can depend on C++20 and use concepts for the task. Example: See the QStringView ctor linked above.
      1. Don't be inventive with the formulation of the note. Use the technocratic phrase above. Rationale: Once we can depend on C++20, we will want to port our constrained templates to concepts and then easy grepability will ensure we don't overlook something.

Relational Operators

Comparison between signed and unsigned integer types

  1. Avoid comparing signed and unsigned integer values directly. Use the safe q20::cmp_* API for it: q20::{cmp_{equal,not_equal,{less,greater}{,_equal}},in_range}. Rationale: Integer comparison may involve implicit conversions that make the result deviate from what would be mathematically correct, e.g. -1 < 0u is false, but q20::cmp_less(-1, 0u) is true, as expected.
    1. Exception: If signed/unsigned values are intended to be converted to the unsigned type and compared like that, then cast the signed type to the unsigned equivalent and not use cmp_{*} functions.
  2. Never convert the unsigned value to a signed type and compare in the signed domain. Rationale: It's simpler and safer to compare the signed value to zero (0) and then cast to unsigned for the comparison with the other value, because converting an unsigned value to signed may make it negative, causing false positives.

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 can't be moved, and its cleanup() protocol 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.
  7. QSharedDataPointer → QExplicitlySharedDataPoiner Rationale: QSDP performs an atomic check on each access and may detach prematurely. QESDP's detach is explicit (hence the name) and therefore it doesn't permanently check the ref-count. Caveats: When porting to QESDP, need to carefully add all the detach() calls, and also observe that the two classes have different const-propagation (one is deep, one is shallow).
  8. q(v)nprintf() → std::(v)snprinf() (not snprintf() from stdio.h; you must #include <cstdio>!) (QT_NO_QSNPRINTF) Rationale: qsnprintf() exhibits even more platform-dependent behaviour than C++11 std::snprintf() does. At least std::snprinf() is now C++-standardized and MSVC, which ignores C standards > 89, implements it without security warnings and with C99 semantics. qsnprintf(), OTOH, has nasty fall-backs to QString::asprintf().toLocal8Bit() (which treats %ls and %a differently) or to snprintf_s() (which differs in return values).

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.

QExplicitlySharedDataPointer

  1. When using it as a pimpl-ptr, consider using the QT_DECLARE_QESDP_SPECIALIZATION_DTOR and QT_DEFINE_QESDP_SPECIALIZATION_DTOR macros. Rationale: Allows to make the move-constructor of the Public class inline noexcept = defaulted.
    1. Don't use the _WITH_EXPORT version of the macros. Rationale: This is only needed if you wish to also = default the destructor of the Public class inline. The move-ctor needs to have the definition ready, but then doesn't call it. Usually, the destructor of the Public class is = defaulted out-of-line, so non-exported works. When you do use the _WITH_EXPORT macro, you need to add a suppression ELFVERSION:ignore-next to the line before the Private class definition. Rationale: tbd. Ask Thiago in the meantime.
  2. Beware of using Q_D and detach() in one function: after detach(), the d declared by Q_D will still point to the old Private object. Mechanics: Use Q_D as usual, but switch to explicit d_func() after the detach(). We're currently looking into how to make this safer, by adding variants of the Q_D macro.

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::operator*() 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))             // 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.
  6. Do not put code into removed_api.cpp that is any more complex than straightforwardly calling the replacement function. Rationale: The removed_api.cpp code is compiled in, but not callable through the current API. Only code compiled against an older version of Qt will continue to call these function. As a consequence, removed_api.cpp has zero test coverage. The code in there must fulfill "compiles == works". If that cannot be guaranteed (or a reviewer has his doubts), you need to factor the implementation into a new private function and call that from the old and the replacement public function.

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

Axivion / Clazy Checks

This section helps to ensure homogeneous implementation of Axivion and Clazy static-analyzer complaint fixes, by giving guidelines for fixes. Not all checks that were useful once are still useful, some may only apply to new code and not old, etc.

Keep this list sorted by title.

clazy-function-args-by-ref

  1. This can be mostly ignored. Rationale: See #clazy-function-args-by-value. There may be obvious hits like passing QString by value, but even that may be intended, so the first reaction should always be to trust the programmer that used pass-by-value. Only if it turns out that they decided wrongly should a fix be made.
  2. Do not add suppressions. Rationale: This check is fundamentally wrong (too many False Positives), so it should be excluded project-wide, not suppressed on a per-use basis.

clazy-function-args-by-value

  1. Fix in old and new code, ignore the "sure that value would be passed in CPU registers" part. Rationale: Passing by value is always correct (assuming class authors do their job and disable copying when it wouldn't be safe, e.g. in RAII classes or polymorphic class hierarchies (slicing)), but it may be a bit slower (or not, depends on many things). OTOH, pass-by-cref has the problem that the argument may alias another object reachable from the function (e.g. a global or member object). The compiler will have to take that into account, and cannot optimize as aggressively, but more importantly, the author of the code also has to keep that in mind, and that can fail: Example: https://codereview.qt-project.org/c/qt/qtbase/+/191706 References: https://www.macieira.org/blog/2012/02/the-value-of-passing-by-value/ (note that this is old, in particular, C++17 added Guaranteed Copy Elision, making pass-by-value an even better idea that it was in C++98/11: even more efficient, and available to non-copy-non-movable(!) types).

clazy-old-style-connect

  1. Do not port existing code. Rationale: Old-style connect() is safe when a signal is emitted during destruction: Since the slot is called using qt_metacall(), a virtual method, it cannot happen that a slot defined on a Derived is called when ~Derived has already executed and only a Base object is left. New-style-connect instead stores a member function pointer and will happily call the method on a partially-destroyed object. As a consequence, as the old code was developed with the old semantics in mind, you will find that you need to do manual disconnect()s in class dtors, and because that doesn't work without having the corresponding Connection object, you need to store that somewhere, too. Also, new-style connects in general generate more code than old-style-connects, up to a few KiB per class ported. Seeing as it produces more soure and more execuable code, and reduces mainainability (we can suddenly forget to add the disconnect to a newly-added connect), this is not a good idea for existing code. The older the code, the worse of an idea this is.

Qt-FunctionArgsByValueRef

See #clazy-function-args-by-ref or #clazy-function-args-by-value. This check will need to be split. Do not add suppressions.

Qt-OldStyleConnect

See #clazy-old-style-connect.

Decided To Keep Here

(make a proper subsection grouping when moving stuff here)

Retrieved from "https://wiki.qt.io/index.php?title=Things_To_Look_Out_For_In_Reviews&oldid=42971"