Things To Look Out For In Reviews

From Qt Wiki
Revision as of 08:37, 12 January 2024 by Marc Mutz (talk | contribs) (→‎Newly-Added Things: Add section on commit messages.)
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.

Canons

Be aware of the following generally-accepted principles:

Newly-Added Things

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

Commit Message

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

API

  • Name static factory member functions create() and non-static factory functions createFoo(). Rationale: For static member functions, the required class name will give sufficient context (QThread::create()). For non-static member functions, the variable name may be something completely different (api.createRequest()). References: https://codereview.qt-project.org/c/qt/qtbase/+/529602/comments/f93def88_ced8e9ca

Public Headers

Includes

  • Include Qt headers always as <QtModule/qheader.h>. Rationale: it's probably the fastest way to include the file while maintaining compatibility with non-per-module-include build systems.
  • Group includes in descending specificity / ascending generality: first, module headers, then dependenee Qt modules, then QtCore headers, then C++ headers, then C headers, then platform / 3rd-party headers. Separate groups with blank lines. In each group, sort alphabetically. Rationale: Like in CMakeLists's SOURCES, or removed_api.cpp, or tests, ordering code minimizes conflicts between independent changes.

Enums

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

New Classes

Documentation

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

Value Classes

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

Polymorphic Classes

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

RAII Classes

(tbd)

Namespaces

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

Class-Specific Usage Guidelines

std::optional

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

Deprecations and Removals

  • Does the documentation reflect the deprecation, and mention the version it is first deprecated for (see also qdoc's \deprecated command? Does it give the user a rationale, as well as a suggestion for alternatives?
  • for QT_REMOVED_SINCE, does the new function contain a "\note In Qt versions prior to 6.x, this function took/was ..."?

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=41264"