CodeStyle.rst (19164B)
1 ==================================== 2 DOM Workers & Storage C++ Code Style 3 ==================================== 4 5 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. 6 7 .. contents:: 8 :depth: 4 9 10 Introduction 11 ============ 12 13 This code style currently applies to the components living in the following directories: 14 15 * ``dom/file`` 16 * ``dom/indexedDB`` 17 * ``dom/localstorage`` 18 * ``dom/payments`` 19 * ``dom/quota`` 20 * ``dom/serviceworkers`` 21 * ``dom/workers`` 22 23 In the long-term, the code is intended to use the 24 :ref:`Mozilla Coding Style <Coding style>`, 25 which references the `Google C++ Coding Style <https://google.github.io/styleguide/cppguide.html>`_. 26 27 However, large parts of the code were written before rules and in particular 28 the reference to the Google C++ Coding Style were enacted, and due to the 29 size of the code, this misalignment cannot be fixed in the short term. 30 To avoid that an arbitrary mixture of old-style and new-style code grows, 31 this document makes deviations from the "global" code style explicit, and 32 will be amended to describe migration paths in the future. 33 34 In addition, to achieve higher consistency within the components maintained by 35 the team and to reduce style discussions during reviews, allowing them to focus 36 on more substantial issues, more specific rules are described here that go 37 beyond the global code style. These topics might have been deliberately or 38 accidentally omitted from the global code style. Depending on wider agreement 39 and applicability, these specific rules might be migrated into the global code 40 style in the future. 41 42 Note that this document does not cover pure formatting issues. The code is and 43 must be kept formatted automatically by clang-format using the supplied 44 configuration file, and whatever clang-format does takes precedence over any 45 other stated rules regarding formatting. 46 47 Deviations from the Google C++ Coding Style 48 =========================================== 49 50 Deviations not documented yet. 51 52 Deviations from the Mozilla C++ Coding Style 53 ============================================ 54 55 .. the table renders impractically, cf. https://github.com/readthedocs/sphinx_rtd_theme/issues/117 56 57 .. tabularcolumns:: |p{4cm}|p{4cm}|p{2cm}|p{2cm}| 58 59 +--------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------+-----------------+-------------------------------------------------------------------------------------+ 60 | Mozilla style | Prevalent WAS style | Deviation scope | Evolution | 61 +========================================================================================================+============================================================================================+=================+=====================================================================================+ 62 | 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 | 63 | | 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. | 64 | | | | | 65 +--------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------+-----------------+-------------------------------------------------------------------------------------+ 66 | `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 | 67 | 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). | 68 | swap(). <https://google.github.io/styleguide/cppguide.html#Reference_Arguments>`_ | | | | 69 +--------------------------------------------------------------------------------------------------------+--------------------------------------------------------------------------------------------+-----------------+-------------------------------------------------------------------------------------+ 70 71 Additions to the Google/Mozilla C++ Code Style 72 ============================================== 73 74 This section contains style guidelines that do not conflict with the Google or 75 Mozilla C++ Code Style, but may make guidelines more specific or add guidelines 76 on topics not covered by those style guides at all. 77 78 Naming 79 ------ 80 81 gtest test names 82 ~~~~~~~~~~~~~~~~ 83 84 gtest constructs a full test name from different fragments. Test names are 85 constructed somewhat differently for basic and parametrized tests. 86 87 The *prefix* for a test should start with an identifier of the component 88 and class, based on the name of the source code directory, transformed to 89 PascalCase and underscores as separators, so e.g. for a class ``Key`` in 90 ``dom/indexedDB``, use ``DOM_IndexedDB_Key`` as a prefix. 91 92 For basic tests constructed with ``TEST(test_case_name, test_name)``: Use 93 the *prefix* as the ``test_case_name``. Test ``test_name`` should start with 94 the name of tested method(s), and a . Use underscores as a separator within 95 the ``test_name``. 96 97 Value-parametrized tests are constructed with 98 ``TEST_P(parametrized_test_case_name, parametrized_test_name)``. They require a 99 custom test base class, whose name is used as the ``parametrized_test_case_name``. 100 Start the class name with ``TestWithParam_``, and end it with a transliteration 101 of the parameter type (e.g. ``String_Int_Pair`` for ``std::pair<nsString, int>``), 102 and place it in an (anonymous) namespace. 103 104 .. attention:: 105 It is important to place the class in an (anonymous) namespace, since its 106 name according to this guideline is not unique within libxul-gtest, and name 107 clashes are likely, which would lead to ODR violations otherwise. 108 109 A ``parametrized_test_name`` is constructed according to the same rules 110 described for ``test_name`` above. 111 112 Instances of value-parametrized tests are constructed using 113 ``INSTANTIATE_TEST_CASE_P(prefix, parametrized_test_case_name, generator, ...)``. 114 As ``prefix``, use the prefix as described above. 115 116 Similar considerations apply to type-parametrized tests. If necessary, specific 117 rules for type-parametrized tests will be added here. 118 119 Rationale 120 All gtests (not only from the WAS components) are linked into libxul-gtest, 121 which requires names to be unique within that large scope. In addition, it 122 should be clear from the test name (e.g. in the test execution log) in what 123 source file (or at least which directory) the test code can be found. 124 Optimally, test names should be structured hierarchically to allow 125 easy selection of groups of tests for execution. However, gtest has some 126 restrictions that do not allow that completely. The guidelines try to 127 accommodate for these as far as possible. Note that gtest recommends not to 128 use underscores in test names in general, because this may lead to reserved 129 names and naming conflicts, but the rules stated here should avoid that. 130 In case of any problems arising, we can evolve the rules to accommodate 131 for that. 132 133 Specifying types 134 ---------------- 135 136 Use of ``auto`` for declaring variables 137 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 138 139 The `Google C++ Code Style on auto <https://google.github.io/styleguide/cppguide.html#auto>`_ 140 allows the use of ``auto`` generally with encouragements for specific cases, which still 141 leaves a rather wide range for interpretation. 142 143 We extend this by some more encouragements and discouragements: 144 145 * DO use ``auto`` when the type is already present in the 146 initialization expression (esp. a template argument or similar), 147 e.g. ``auto c = static_cast<uint16_t>(*(iter++)) << 8;`` or 148 ``auto x = MakeRefPtr<MediaStreamError>(mWindow, *aError);`` 149 150 * DO use ``auto`` if the spelled out type were complex otherwise, 151 e.g. a nested typedef or type alias, e.g. ``foo_container::value_type``. 152 153 * DO NOT use ``auto`` if the type were spelled out as a builtin 154 integer type or one of the types from ``<cstdint>``, e.g. 155 instead of ``auto foo = funcThatReturnsUint16();`` use 156 ``uint16_t foo = funcThatReturnsUint16();``. 157 158 .. note:: 159 Some disadvantages of using ``auto`` relate to the unavailability of type 160 information outside an appropriate IDE/editor. This may be somewhat remedied 161 by resolving `Bug 1567464 <https://bugzilla.mozilla.org/show_bug.cgi?id=1567464>`_ 162 which will make the type information available in searchfox. In consequence, 163 the guidelines might be amended to promote a more widespread use of ``auto``. 164 165 Pointer types 166 ------------- 167 168 Plain pointers 169 ~~~~~~~~~~~~~~ 170 171 The use of plain pointers is error-prone. Avoid using owning plain pointers. In 172 particular, avoid using literal, non-placement new. There are various kinds 173 of smart pointers, not all of which provide appropriate factory functions. 174 However, where such factory functions exist, do use them (along with auto). 175 The following is an incomplete list of smart pointer types and corresponding 176 factory functions: 177 178 +------------------------+-------------------------+------------------------+ 179 | Type | Factory function | Header file | 180 +========================+=========================+========================+ 181 | ``mozilla::RefPtr`` | ``mozilla::MakeRefPtr`` | ``"mfbt/RefPtr.h"`` | 182 +------------------------+-------------------------+------------------------+ 183 | ``mozilla::UniquePtr`` | ``mozilla::MakeUnique`` | ``"mfbt/UniquePtr.h"`` | 184 +------------------------+-------------------------+------------------------+ 185 | ``std::unique_ptr`` | ``std::make_unique`` | ``<memory>`` | 186 +------------------------+-------------------------+------------------------+ 187 | ``std::shared_ptr`` | ``std::make_shared`` | ``<memory>`` | 188 +------------------------+-------------------------+------------------------+ 189 190 Also, to create an ``already_AddRefed<>`` to pass as a parameter or return from 191 a function without the need to dereference it, use ``MakeAndAddRef`` instead of 192 creating a dereferenceable ``RefPtr`` (or similar) first and then using 193 ``.forget()``. 194 195 Smart pointers 196 ~~~~~~~~~~~~~~ 197 198 In function signatures, prefer accepting or returning ``RefPtr`` instead of 199 ``already_AddRefed`` in conjunction with regular ``std::move`` rather than 200 ``.forget()``. This improves readability and code generation. Prevailing 201 legimitate uses of ``already_AddRefed`` are described in its 202 `documentation <https://searchfox.org/mozilla-central/rev/4df8821c1b824db5f40f381f48432f219d99ae36/mfbt/AlreadyAddRefed.h#31>`_. 203 204 Prefer using ``mozilla::UniquePtr`` over ``nsAutoPtr``, since the latter is 205 deprecated (and e.g. has no factory function, see Bug 1600079). 206 207 Use ``nsCOMPtr<T>`` iff ``T`` is an XPCOM interface type 208 (`more details on MDN <https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/nsCOMPtr_versus_RefPtr>`). 209 210 Enums 211 ----- 212 213 Use scoped resp. strongly typed enums (``enum struct``) rather than non-scoped 214 enums. Use PascalCase for naming the values of scoped enums. 215 216 Evolution Process 217 ================= 218 219 This section explains the process to evolve the coding style described in this 220 document. For clarity, we will distinguish coding tasks from code style 221 evolution tasks in this section. 222 223 Managing code style evolution tasks 224 ----------------------------------- 225 226 A code style evolution task is a task that ought to amend or revise the 227 coding style as described in this document. 228 229 Code style evolution tasks should be managed in Bugzilla, as individual bugs 230 for each topic. All such tasks 231 should block the meta-bug 232 `1586788 <https://bugzilla.mozilla.org/show_bug.cgi?id=1586788>`. 233 234 When you take on to work on a code style evolution task: 235 236 - The task may already include a sketch of a resolution. If no preferred 237 solution is obvious, discuss options to resolve it via comments on the bug 238 first. 239 - When the general idea is ready to be spelled out in this document, amend or 240 revise it accordingly. 241 - Submit the changes to this document as a patch to Phabricator, and put it up 242 for review. Since this will affect a number of people, every change should 243 be reviewed by at least two people. Ideally, this should include the owner 244 of this style document and one person with good knowledge of the parts of 245 the code base this style applies to. 246 - If there are known violations of the amendment to the coding style, consider 247 fixing some of them, so that the amendment is tested on actual code. If 248 the code style evolution task refers to a particular code location from a 249 review, at least that location should be fixed to comply with the amended 250 coding style. 251 - When you have two r+, land the patch. 252 - Report on the addition in the next team meeting to raise awareness. 253 254 Basis for code style evolution tasks 255 ------------------------------------ 256 257 The desire or necessity to evolve the code style can originate from 258 different activities, including 259 - reviews 260 - reading or writing code locally 261 - reading the coding style 262 - general thoughts on coding style 263 264 The code style should not be cluttered with aspects that are rarely 265 relevant or rarely leads to discussions, as the maintenance of the 266 code style has a cost as well. The code style should be as comprehensive 267 as necessary to reduce the overall maintenance costs of the code and 268 code style combined. 269 270 A particular focus is therefore on aspects that led to some discussion in 271 a code review, as reducing the number or verbosity of necessary style 272 discussions in reviews is a major indicator for the effectiveness of the 273 documented style. 274 275 Evolving code style based on reviews 276 ------------------------------------ 277 278 The goal of the process described here is to take advantage of style-related 279 discussions that originate from a code review, but to decouple evolution of 280 the code style from the review process, so that it does not block progress on 281 the underlying bug. 282 283 The following should be considered when performing a review: 284 285 - Remind yourself of the code style, maybe skim through the document before 286 starting the review, or have it open side-by-side while doing the review. 287 - If you find a violation of an existing rule, add an inline comment. 288 - Have an eye on style-relevant aspects in the code itself or after a 289 discussions with the author. Consider if this could be generalized into a 290 style rule, but is not yet covered by the documented global or local style. 291 This might be something that is in a different style as opposed to other 292 locations, differs from your personal style, etc. 293 - In that case, find an acceptable temporary solution for the code fragments 294 at hand, which is acceptable for an r+ of the patch. Maybe agree with the 295 code author on adding a comment that this should be revised later, when 296 a rule is codified. 297 - Create a code style evolution task in Bugzilla as described above. In the 298 description of the bug, reference the review comment that gave rise to it. 299 If you can suggest a resolution, include that in the description, but this 300 is not a necessary condition for creating the task. 301 302 Improving code style compliance when writing code 303 ------------------------------------------------- 304 305 Periodically look into the code style document, and remind yourself of its 306 rules, and give particular attention to recent changes. 307 308 When writing code, i.e. adding new code or modify existing code, 309 remind yourself of checking the code for style compliance. 310 311 Time permitting, resolve existing violations on-the-go as part of other work 312 in the code area. Submit such changes in dedicated patches. If you identify 313 major violations that are too complex to resolve on-the-go, consider 314 creating a bug dedicated to the resolution of that violation, which 315 then can be scheduled in the planning process. 316 317 Syncing with the global Mozilla C++ Coding Style 318 ------------------------------------------------ 319 320 Several aspects of the coding style described here will be applicable to 321 the overall code base. However, amendments to the global coding style will 322 affect a large number of code authors and may require extended discussion. 323 Deviations from the global coding style should be limited in the long term. 324 On the other hand, amendments that are not relevant to all parts of the code 325 base, or where it is difficult to reach a consensus at the global scope, 326 may make sense to be kept in the local style. 327 328 The details of synchronizing with the global style are subject to discussion 329 with the owner and peers of the global coding style (see 330 `Bug 1587810 <https://bugzilla.mozilla.org/show_bug.cgi?id=1587810>`). 331 332 FAQ 333 --- 334 335 * When someone introduces new code that adheres to the current style, but the 336 remainder of the function/class/file does not, is it their responsibility 337 to update that remainder on-the-go? 338 339 The code author is not obliged to update the remainder, but they are 340 encouraged to do so, time permitting. Whether that is the case depends on a 341 number of factors, including the number and complexity of existing style 342 violations, the risk introduced by changing that on the go etc. Judging this 343 is left to the code author. 344 At the very least, the function/class/file should not be left in a worse 345 state than before. 346 347 * Are stylistic inconsistencies introduced by applying the style as defined 348 here only to new code considered acceptable? 349 350 While this is certainly not optimal, accepting such inconsistencies to 351 some degree is inevitable to allow making progress towards an improved style. 352 Personal preferences regarding the degree may differ, but in doubt such 353 inconsistencies should be considered acceptable. They should not block a bug 354 from being closed.