Things To Look Out For In Reviews

From Qt Wiki
Revision as of 17:29, 9 January 2024 by Marc Mutz (talk | contribs) (→‎New Classes: Q_DECLARE_SHARED, Q_DECLARE_METATYPE, Rationale for not using QSDP.)
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.

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

New 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.
    • Don't add Q_DECLARE_METATYPE. Rationale: it's automatic now.
  • 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?

Polymorphic Classes

Deprecated and Removed Things

  • 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?

Decided To Keep Here

(make a proper subsection grouping when moving stuff here)