path: root/dom/docs/workersAndStorage/CodeStyle.rst

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