Things To Look Out For In Reviews: Difference between revisions
Jump to navigation
Jump to search
(Commit Policy is another potential destination for these.) |
(Added documentation hints) |
||
| Line 13: | Line 13: | ||
* 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/ | * 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) | * 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 enums 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 Classes == | == New Classes == | ||
Make sure new classes follow the [https://codereview.qt-project.org/c/meta/quips/+/321050 draft value-class mechanics QUIP-22]. | * Make sure new classes follow the [https://codereview.qt-project.org/c/meta/quips/+/321050 draft value-class mechanics QUIP-22]. | ||
* Documentation: | |||
** Is the documentation for the class complete and in good shape (grammar, style, correctness)? | |||
** Is the class documented as new with <syntaxhighlight inline>\since</syntaxhighlight>? | |||
** 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 <syntaxhighlight inline>\ingroup</syntaxhighlight> commands and whether it makes sense the class should also have it! | |||
** Should the class be mentioned in the [https://doc-snapshots.qt.io/qt6-dev/whatsnewqt6.html What's New in Qt 6] overview documentation? | |||
=== Polymorphic Classes === | === 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 | - 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 | ||
= Deprecated and Removed Things = | |||
* Does the documentation reflect the deprecation, and mention the version it is first deprecated for (see also qdoc's [https://doc.qt.io/qt-6/16-qdoc-commands-status.html#deprecated-command \deprecated] command? Does it give the user a rationale, as well as a suggestion for alternatives? | |||
= Decided To Keep Here = | = Decided To Keep Here = | ||
(make a proper subsection grouping when moving stuff here) | (make a proper subsection grouping when moving stuff here) | ||
Revision as of 09:38, 5 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.
Newly-Added Things
When adding a new Thing, don't forget to provide rationale.
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 enums should be documented as new for version VERSION with
\value [since VERSION]See also qdoc's \value command
New Classes
- Make sure new classes follow the draft value-class mechanics QUIP-22.
- 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
\ingroupcommands 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
- 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
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)