Things To Look Out For In Reviews: Difference between revisions

From Qt Wiki
Jump to navigation Jump to search
(→‎New Classes: drag documentation out of Value Classes into it's own subsubsection)
(→‎Enums: Type of enum; 0 should mean "default".)
Line 35: Line 35:
* 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)
* 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 <syntaxhighlight inline>\value [since VERSION]</syntaxhighlight> See also qdoc's [https://doc.qt.io/qt-6/10-qdoc-commands-tablesandlists.html#value-command \value command]
* New enumerators should be documented as new for version VERSION with <syntaxhighlight inline>\value [since VERSION]</syntaxhighlight> See also qdoc's [https://doc.qt.io/qt-6/10-qdoc-commands-tablesandlists.html#value-command \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 ==
== New Classes ==

Revision as of 09:01, 10 January 2024


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

  • 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

Polymorphic Classes

RAII Classes

(tbd)

Namespaces

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

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)