summaryrefslogtreecommitdiffstats
path: root/dom/docs/workersAndStorage/CodeStyle.rst
diff options
context:
space:
mode:
authorDaniel Baumann <daniel.baumann@progress-linux.org>2024-04-07 17:32:43 +0000
committerDaniel Baumann <daniel.baumann@progress-linux.org>2024-04-07 17:32:43 +0000
commit6bf0a5cb5034a7e684dcc3500e841785237ce2dd (patch)
treea68f146d7fa01f0134297619fbe7e33db084e0aa /dom/docs/workersAndStorage/CodeStyle.rst
parentInitial commit. (diff)
downloadthunderbird-6bf0a5cb5034a7e684dcc3500e841785237ce2dd.tar.xz
thunderbird-6bf0a5cb5034a7e684dcc3500e841785237ce2dd.zip
Adding upstream version 1:115.7.0.upstream/1%115.7.0upstream
Signed-off-by: Daniel Baumann <daniel.baumann@progress-linux.org>
Diffstat (limited to 'dom/docs/workersAndStorage/CodeStyle.rst')
-rw-r--r--dom/docs/workersAndStorage/CodeStyle.rst354
1 files changed, 354 insertions, 0 deletions
diff --git a/dom/docs/workersAndStorage/CodeStyle.rst b/dom/docs/workersAndStorage/CodeStyle.rst
new file mode 100644
index 0000000000..822779d179
--- /dev/null
+++ b/dom/docs/workersAndStorage/CodeStyle.rst
@@ -0,0 +1,354 @@
+====================================
+DOM Workers & Storage C++ Code Style
+====================================
+
+This page describes the code style for the components maintained by the DOM Workers & Storage team. They live in-tree under the 'dom/docs/indexedDB' directory.
+
+.. contents::
+ :depth: 4
+
+Introduction
+============
+
+This code style currently applies to the components living in the following directories:
+
+* ``dom/file``
+* ``dom/indexedDB``
+* ``dom/localstorage``
+* ``dom/payments``
+* ``dom/quota``
+* ``dom/serviceworkers``
+* ``dom/workers``
+
+In the long-term, the code is intended to use the
+:ref:`Mozilla Coding Style <Coding style>`,
+which references the `Google C++ Coding Style <https://google.github.io/styleguide/cppguide.html>`_.
+
+However, large parts of the code were written before rules and in particular
+the reference to the Google C++ Coding Style were enacted, and due to the
+size of the code, this misalignment cannot be fixed in the short term.
+To avoid that an arbitrary mixture of old-style and new-style code grows,
+this document makes deviations from the "global" code style explicit, and
+will be amended to describe migration paths in the future.
+
+In addition, to achieve higher consistency within the components maintained by
+the team and to reduce style discussions during reviews, allowing them to focus
+on more substantial issues, more specific rules are described here that go
+beyond the global code style. These topics might have been deliberately or
+accidentally omitted from the global code style. Depending on wider agreement
+and applicability, these specific rules might be migrated into the global code
+style in the future.
+
+Note that this document does not cover pure formatting issues. The code is and
+must be kept formatted automatically by clang-format using the supplied
+configuration file, and whatever clang-format does takes precedence over any
+other stated rules regarding formatting.
+
+Deviations from the Google C++ Coding Style
+===========================================
+
+Deviations not documented yet.
+
+Deviations from the Mozilla C++ Coding Style
+============================================
+
+.. the table renders impractically, cf. https://github.com/readthedocs/sphinx_rtd_theme/issues/117
+
+.. tabularcolumns:: |p{4cm}|p{4cm}|p{2cm}|p{2cm}|
+
++--------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------+-----------------+-------------------------------------------------------------------------------------+
+| Mozilla style | Prevalent WAS style | Deviation scope | Evolution |
++========================================================================================================+============================================================================================+=================+=====================================================================================+
+| We prefer using "static", instead of anonymous C++ namespaces. | Place all symbols that should have internal linkage in a single anonymous | All files | Unclear. The recommendation in the Mozilla code style says this might change in the |
+| | namespace block at the top of an implementation file, rather than declarating them static. | | future depending on debugger support, so this deviation might become obsolete. |
+| | | | |
++--------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------+-----------------+-------------------------------------------------------------------------------------+
+| `All parameters passed by lvalue reference must be labeled const. [...] Input parameters may be const | Non-const reference parameters may be used. | All files | Unclear. Maybe at least restrict the use of non-const reference parameters to |
+| pointers, but we never allow non-const reference parameters except when required by convention, e.g., | | | cases that are not clearly output parameters (i.e. which are assigned to). |
+| swap(). <https://google.github.io/styleguide/cppguide.html#Reference_Arguments>`_ | | | |
++--------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------+-----------------+-------------------------------------------------------------------------------------+
+
+Additions to the Google/Mozilla C++ Code Style
+==============================================
+
+This section contains style guidelines that do not conflict with the Google or
+Mozilla C++ Code Style, but may make guidelines more specific or add guidelines
+on topics not covered by those style guides at all.
+
+Naming
+------
+
+gtest test names
+~~~~~~~~~~~~~~~~
+
+gtest constructs a full test name from different fragments. Test names are
+constructed somewhat differently for basic and parametrized tests.
+
+The *prefix* for a test should start with an identifier of the component
+and class, based on the name of the source code directory, transformed to
+PascalCase and underscores as separators, so e.g. for a class ``Key`` in
+``dom/indexedDB``, use ``DOM_IndexedDB_Key`` as a prefix.
+
+For basic tests constructed with ``TEST(test_case_name, test_name)``: Use
+the *prefix* as the ``test_case_name``. Test ``test_name`` should start with
+the name of tested method(s), and a . Use underscores as a separator within
+the ``test_name``.
+
+Value-parametrized tests are constructed with
+``TEST_P(parametrized_test_case_name, parametrized_test_name)``. They require a
+custom test base class, whose name is used as the ``parametrized_test_case_name``.
+Start the class name with ``TestWithParam_``, and end it with a transliteration
+of the parameter type (e.g. ``String_Int_Pair`` for ``std::pair<nsString, int>``),
+and place it in an (anonymous) namespace.
+
+.. attention::
+ It is important to place the class in an (anonymous) namespace, since its
+ name according to this guideline is not unique within libxul-gtest, and name
+ clashes are likely, which would lead to ODR violations otherwise.
+
+A ``parametrized_test_name`` is constructed according to the same rules
+described for ``test_name`` above.
+
+Instances of value-parametrized tests are constructed using
+``INSTANTIATE_TEST_CASE_P(prefix, parametrized_test_case_name, generator, ...)``.
+As ``prefix``, use the prefix as described above.
+
+Similar considerations apply to type-parametrized tests. If necessary, specific
+rules for type-parametrized tests will be added here.
+
+Rationale
+ All gtests (not only from the WAS components) are linked into libxul-gtest,
+ which requires names to be unique within that large scope. In addition, it
+ should be clear from the test name (e.g. in the test execution log) in what
+ source file (or at least which directory) the test code can be found.
+ Optimally, test names should be structured hierarchically to allow
+ easy selection of groups of tests for execution. However, gtest has some
+ restrictions that do not allow that completely. The guidelines try to
+ accommodate for these as far as possible. Note that gtest recommends not to
+ use underscores in test names in general, because this may lead to reserved
+ names and naming conflicts, but the rules stated here should avoid that.
+ In case of any problems arising, we can evolve the rules to accommodate
+ for that.
+
+Specifying types
+----------------
+
+Use of ``auto`` for declaring variables
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+The `Google C++ Code Style on auto <https://google.github.io/styleguide/cppguide.html#auto>`_
+allows the use of ``auto`` generally with encouragements for specific cases, which still
+leaves a rather wide range for interpretation.
+
+We extend this by some more encouragements and discouragements:
+
+* DO use ``auto`` when the type is already present in the
+ initialization expression (esp. a template argument or similar),
+ e.g. ``auto c = static_cast<uint16_t>(*(iter++)) << 8;`` or
+ ``auto x = MakeRefPtr<MediaStreamError>(mWindow, *aError);``
+
+* DO use ``auto`` if the spelled out type were complex otherwise,
+ e.g. a nested typedef or type alias, e.g. ``foo_container::value_type``.
+
+* DO NOT use ``auto`` if the type were spelled out as a builtin
+ integer type or one of the types from ``<cstdint>``, e.g.
+ instead of ``auto foo = funcThatReturnsUint16();`` use
+ ``uint16_t foo = funcThatReturnsUint16();``.
+
+.. note::
+ Some disadvantages of using ``auto`` relate to the unavailability of type
+ information outside an appropriate IDE/editor. This may be somewhat remedied
+ by resolving `Bug 1567464 <https://bugzilla.mozilla.org/show_bug.cgi?id=1567464>`_
+ which will make the type information available in searchfox. In consequence,
+ the guidelines might be amended to promote a more widespread use of ``auto``.
+
+Pointer types
+-------------
+
+Plain pointers
+~~~~~~~~~~~~~~
+
+The use of plain pointers is error-prone. Avoid using owning plain pointers. In
+particular, avoid using literal, non-placement new. There are various kinds
+of smart pointers, not all of which provide appropriate factory functions.
+However, where such factory functions exist, do use them (along with auto).
+The following is an incomplete list of smart pointer types and corresponding
+factory functions:
+
++------------------------+-------------------------+------------------------+
+| Type | Factory function | Header file |
++========================+=========================+========================+
+| ``mozilla::RefPtr`` | ``mozilla::MakeRefPtr`` | ``"mfbt/RefPtr.h"`` |
++------------------------+-------------------------+------------------------+
+| ``mozilla::UniquePtr`` | ``mozilla::MakeUnique`` | ``"mfbt/UniquePtr.h"`` |
++------------------------+-------------------------+------------------------+
+| ``std::unique_ptr`` | ``std::make_unique`` | ``<memory>`` |
++------------------------+-------------------------+------------------------+
+| ``std::shared_ptr`` | ``std::make_shared`` | ``<memory>`` |
++------------------------+-------------------------+------------------------+
+
+Also, to create an ``already_AddRefed<>`` to pass as a parameter or return from
+a function without the need to dereference it, use ``MakeAndAddRef`` instead of
+creating a dereferenceable ``RefPtr`` (or similar) first and then using
+``.forget()``.
+
+Smart pointers
+~~~~~~~~~~~~~~
+
+In function signatures, prefer accepting or returning ``RefPtr`` instead of
+``already_AddRefed`` in conjunction with regular ``std::move`` rather than
+``.forget()``. This improves readability and code generation. Prevailing
+legimitate uses of ``already_AddRefed`` are described in its
+`documentation <https://searchfox.org/mozilla-central/rev/4df8821c1b824db5f40f381f48432f219d99ae36/mfbt/AlreadyAddRefed.h#31>`_.
+
+Prefer using ``mozilla::UniquePtr`` over ``nsAutoPtr``, since the latter is
+deprecated (and e.g. has no factory function, see Bug 1600079).
+
+Use ``nsCOMPtr<T>`` iff ``T`` is an XPCOM interface type
+(`more details on MDN <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/nsCOMPtr_versus_RefPtr>`).
+
+Enums
+-----
+
+Use scoped resp. strongly typed enums (``enum struct``) rather than non-scoped
+enums. Use PascalCase for naming the values of scoped enums.
+
+Evolution Process
+=================
+
+This section explains the process to evolve the coding style described in this
+document. For clarity, we will distinguish coding tasks from code style
+evolution tasks in this section.
+
+Managing code style evolution tasks
+-----------------------------------
+
+A code style evolution task is a task that ought to amend or revise the
+coding style as described in this document.
+
+Code style evolution tasks should be managed in Bugzilla, as individual bugs
+for each topic. All such tasks
+should block the meta-bug
+`1586788 <https://bugzilla.mozilla.org/show_bug.cgi?id=1586788>`.
+
+When you take on to work on a code style evolution task:
+
+- The task may already include a sketch of a resolution. If no preferred
+ solution is obvious, discuss options to resolve it via comments on the bug
+ first.
+- When the general idea is ready to be spelled out in this document, amend or
+ revise it accordingly.
+- Submit the changes to this document as a patch to Phabricator, and put it up
+ for review. Since this will affect a number of people, every change should
+ be reviewed by at least two people. Ideally, this should include the owner
+ of this style document and one person with good knowledge of the parts of
+ the code base this style applies to.
+- If there are known violations of the amendment to the coding style, consider
+ fixing some of them, so that the amendment is tested on actual code. If
+ the code style evolution task refers to a particular code location from a
+ review, at least that location should be fixed to comply with the amended
+ coding style.
+- When you have two r+, land the patch.
+- Report on the addition in the next team meeting to raise awareness.
+
+Basis for code style evolution tasks
+------------------------------------
+
+The desire or necessity to evolve the code style can originate from
+different activities, including
+- reviews
+- reading or writing code locally
+- reading the coding style
+- general thoughts on coding style
+
+The code style should not be cluttered with aspects that are rarely
+relevant or rarely leads to discussions, as the maintenance of the
+code style has a cost as well. The code style should be as comprehensive
+as necessary to reduce the overall maintenance costs of the code and
+code style combined.
+
+A particular focus is therefore on aspects that led to some discussion in
+a code review, as reducing the number or verbosity of necessary style
+discussions in reviews is a major indicator for the effectiveness of the
+documented style.
+
+Evolving code style based on reviews
+------------------------------------
+
+The goal of the process described here is to take advantage of style-related
+discussions that originate from a code review, but to decouple evolution of
+the code style from the review process, so that it does not block progress on
+the underlying bug.
+
+The following should be considered when performing a review:
+
+- Remind yourself of the code style, maybe skim through the document before
+ starting the review, or have it open side-by-side while doing the review.
+- If you find a violation of an existing rule, add an inline comment.
+- Have an eye on style-relevant aspects in the code itself or after a
+ discussions with the author. Consider if this could be generalized into a
+ style rule, but is not yet covered by the documented global or local style.
+ This might be something that is in a different style as opposed to other
+ locations, differs from your personal style, etc.
+- In that case, find an acceptable temporary solution for the code fragments
+ at hand, which is acceptable for an r+ of the patch. Maybe agree with the
+ code author on adding a comment that this should be revised later, when
+ a rule is codified.
+- Create a code style evolution task in Bugzilla as described above. In the
+ description of the bug, reference the review comment that gave rise to it.
+ If you can suggest a resolution, include that in the description, but this
+ is not a necessary condition for creating the task.
+
+Improving code style compliance when writing code
+-------------------------------------------------
+
+Periodically look into the code style document, and remind yourself of its
+rules, and give particular attention to recent changes.
+
+When writing code, i.e. adding new code or modify existing code,
+remind yourself of checking the code for style compliance.
+
+Time permitting, resolve existing violations on-the-go as part of other work
+in the code area. Submit such changes in dedicated patches. If you identify
+major violations that are too complex to resolve on-the-go, consider
+creating a bug dedicated to the resolution of that violation, which
+then can be scheduled in the planning process.
+
+Syncing with the global Mozilla C++ Coding Style
+------------------------------------------------
+
+Several aspects of the coding style described here will be applicable to
+the overall code base. However, amendments to the global coding style will
+affect a large number of code authors and may require extended discussion.
+Deviations from the global coding style should be limited in the long term.
+On the other hand, amendments that are not relevant to all parts of the code
+base, or where it is difficult to reach a consensus at the global scope,
+may make sense to be kept in the local style.
+
+The details of synchronizing with the global style are subject to discussion
+with the owner and peers of the global coding style (see
+`Bug 1587810 <https://bugzilla.mozilla.org/show_bug.cgi?id=1587810>`).
+
+FAQ
+---
+
+* When someone introduces new code that adheres to the current style, but the
+ remainder of the function/class/file does not, is it their responsibility
+ to update that remainder on-the-go?
+
+ The code author is not obliged to update the remainder, but they are
+ encouraged to do so, time permitting. Whether that is the case depends on a
+ number of factors, including the number and complexity of existing style
+ violations, the risk introduced by changing that on the go etc. Judging this
+ is left to the code author.
+ At the very least, the function/class/file should not be left in a worse
+ state than before.
+
+* Are stylistic inconsistencies introduced by applying the style as defined
+ here only to new code considered acceptable?
+
+ While this is certainly not optimal, accepting such inconsistencies to
+ some degree is inevitable to allow making progress towards an improved style.
+ Personal preferences regarding the degree may differ, but in doubt such
+ inconsistencies should be considered acceptable. They should not block a bug
+ from being closed.