Things To Look Out For In Reviews

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:

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

Newly-Added Things

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

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:topic 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.

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

API

1. 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
2. Don't default arguments unless it's of Trivial Type. Use out-of-line overloading instead. Rationale: Avoids expensive construction of these object 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.

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

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

Classes with QProperty

1. New classes that have Q_PROPERTY declarations, the QPROPERTY should be marked FINAL if it's not intended to be overriden; for already existing classes adding FINAL breaks binary compatibility. Rationale: FINAL properties result in a significant performance boost for QML. See this thread on the devel ML.

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

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.

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 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.
   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 sapping 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.
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. For QObject subclasses, 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. 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). It 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.
   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.

RAII Classes

1. Observe QUIP-19 (nodiscard policy). TL;DR: make RAII class ctors [[nodiscard]]

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.

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

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.

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's cleanup() 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.

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

---

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.

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

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.

Decided To Keep Here

(make a proper subsection grouping when moving stuff here)