1================ 2C++ Coding style 3================ 4 5 6This document attempts to explain the basic styles and patterns used in 7the Mozilla codebase. New code should try to conform to these standards, 8so it is as easy to maintain as existing code. There are exceptions, but 9it's still important to know the rules! 10 11This article is particularly for those new to the Mozilla codebase, and 12in the process of getting their code reviewed. Before requesting a 13review, please read over this document, making sure that your code 14conforms to recommendations. 15 16.. container:: blockIndicator warning 17 18 The Firefox code base adopts parts of the `Google Coding style for C++ 19 code <https://google.github.io/styleguide/cppguide.html>`__, but not all of its rules. 20 A few rules are followed across the code base, others are intended to be 21 followed in new or significantly revised code. We may extend this list in the 22 future, when we evaluate the Google Coding Style for C++ Code further and/or update 23 our coding practices. However, the plan is not to adopt all rules of the Google Coding 24 Style for C++ Code. Some rules are explicitly unlikely to be adopted at any time. 25 26 Followed across the code base: 27 28 - `Formatting <https://google.github.io/styleguide/cppguide.html#Formatting>`__, 29 except for subsections noted here otherwise 30 - `Implicit Conversions <https://google.github.io/styleguide/cppguide.html#Implicit_Conversions>`__, 31 which is enforced by a custom clang-plugin check, unless explicitly overridden using 32 ``MOZ_IMPLICIT`` 33 34 Followed in new/significantly revised code: 35 36 - `Include guards <https://google.github.io/styleguide/cppguide.html#The__define_Guard>`__ 37 38 Unlikely to be ever adopted: 39 40 - `Forward declarations <https://google.github.io/styleguide/cppguide.html#Forward_Declarations>`__ 41 - `Formatting/Conditionals <https://google.github.io/styleguide/cppguide.html#Conditionals>`__ 42 w.r.t. curly braces around inner statements, we require them in all cases where the 43 Google style allows to leave them out for single-line conditional statements 44 45 This list reflects the state of the Google Google Coding Style for C++ Code as of 46 2020-07-17. It may become invalid when the Google modifies its Coding Style. 47 48 49Formatting code 50--------------- 51 52Formatting is done automatically via clang-format, and controlled via in-tree 53configuration files. See :ref:`Formatting C++ Code With clang-format` 54for more information. 55 56Unix-style linebreaks (``\n``), not Windows-style (``\r\n``). You can 57convert patches, with DOS newlines to Unix via the ``dos2unix`` utility, 58or your favorite text editor. 59 60Static analysis 61--------------- 62 63Several of the rules in the Google C++ coding styles and the additions mentioned below 64can be checked via clang-tidy (some rules are from the upstream clang-tidy, some are 65provided via a mozilla-specific plugin). Some of these checks also allow fixes to 66be automatically applied. 67 68``mach static-analysis`` provides a convenient way to run these checks. For example, 69for the check called ``google-readability-braces-around-statements``, you can run: 70 71.. code-block:: shell 72 73 ./mach static-analysis check --checks="-*,google-readability-braces-around-statements" --fix <file> 74 75It may be necessary to reformat the files after automatically applying fixes, see 76:ref:`Formatting C++ Code With clang-format`. 77 78Additional rules 79---------------- 80 81*The norms in this section should be followed for new code. For existing code, 82use the prevailing style in a file or module, ask the owner if you are 83in another team's codebase or it's not clear what style to use.* 84 85 86 87 88Control structures 89~~~~~~~~~~~~~~~~~~ 90 91Always brace controlled statements, even a single-line consequent of 92``if else else``. This is redundant, typically, but it avoids dangling 93else bugs, so it's safer at scale than fine-tuning. 94 95Examples: 96 97.. code-block:: cpp 98 99 if (...) { 100 } else if (...) { 101 } else { 102 } 103 104 while (...) { 105 } 106 107 do { 108 } while (...); 109 110 for (...; ...; ...) { 111 } 112 113 switch (...) { 114 case 1: { 115 // When you need to declare a variable in a switch, put the block in braces. 116 int var; 117 break; 118 } 119 case 2: 120 ... 121 break; 122 default: 123 break; 124 } 125 126``else`` should only ever be followed by ``{`` or ``if``; i.e., other 127control keywords are not allowed and should be placed inside braces. 128 129.. note:: 130 131 For this rule, clang-tidy provides the ``google-readability-braces-around-statements`` 132 check with autofixes. 133 134 135C++ namespaces 136~~~~~~~~~~~~~~ 137 138Mozilla project C++ declarations should be in the ``mozilla`` 139namespace. Modules should avoid adding nested namespaces under 140``mozilla``, unless they are meant to contain names which have a high 141probability of colliding with other names in the code base. For example, 142``Point``, ``Path``, etc. Such symbols can be put under 143module-specific namespaces, under ``mozilla``, with short 144all-lowercase names. Other global namespaces besides ``mozilla`` are 145not allowed. 146 147No ``using`` directives are allowed in header files, except inside class 148definitions or functions. (We don't want to pollute the global scope of 149compilation units that use the header file.) 150 151.. note:: 152 153 For parts of this rule, clang-tidy provides the ``google-global-names-in-headers`` 154 check. It only detects ``using namespace`` directives in the global namespace. 155 156 157``using namespace ...;`` is only allowed in ``.cpp`` files after all 158``#include``\ s. Prefer to wrap code in ``namespace ... { ... };`` 159instead, if possible. ``using namespace ...;``\ should always specify 160the fully qualified namespace. That is, to use ``Foo::Bar`` do not 161write ``using namespace Foo; using namespace Bar;``, write 162``using namespace Foo::Bar;`` 163 164Use nested namespaces (ex: ``namespace mozilla::widget {`` 165 166.. note:: 167 168 clang-tidy provides the ``modernize-concat-nested-namespaces`` 169 check with autofixes. 170 171 172Anonymous namespaces 173~~~~~~~~~~~~~~~~~~~~ 174 175We prefer using ``static``, instead of anonymous C++ namespaces. This may 176change once there is better debugger support (especially on Windows) for 177placing breakpoints, etc. on code in anonymous namespaces. You may still 178use anonymous namespaces for things that can't be hidden with ``static``, 179such as types, or certain objects which need to be passed to template 180functions. 181 182 183C++ classes 184~~~~~~~~~~~~ 185 186.. code-block:: cpp 187 188 namespace mozilla { 189 190 class MyClass : public A 191 { 192 ... 193 }; 194 195 class MyClass 196 : public X 197 , public Y 198 { 199 public: 200 MyClass(int aVar, int aVar2) 201 : mVar(aVar) 202 , mVar2(aVar2) 203 { 204 ... 205 } 206 207 // Special member functions, like constructors, that have default bodies 208 // should use '= default' annotation instead. 209 MyClass() = default; 210 211 // Unless it's a copy or move constructor or you have a specific reason to allow 212 // implicit conversions, mark all single-argument constructors explicit. 213 explicit MyClass(OtherClass aArg) 214 { 215 ... 216 } 217 218 // This constructor can also take a single argument, so it also needs to be marked 219 // explicit. 220 explicit MyClass(OtherClass aArg, AnotherClass aArg2 = AnotherClass()) 221 { 222 ... 223 } 224 225 int LargerFunction() 226 { 227 ... 228 ... 229 } 230 231 private: 232 int mVar; 233 }; 234 235 } // namespace mozilla 236 237Define classes using the style given above. 238 239.. note:: 240 241 For the rule on ``= default``, clang-tidy provides the ``modernize-use-default`` 242 check with autofixes. 243 244 For the rule on explicit constructors and conversion operators, clang-tidy 245 provides the ``mozilla-implicit-constructor`` check. 246 247Existing classes in the global namespace are named with a short prefix 248(For example, ``ns``) as a pseudo-namespace. 249 250 251Methods and functions 252~~~~~~~~~~~~~~~~~~~~~ 253 254 255C/C++ 256^^^^^ 257 258In C/C++, method names should use ``UpperCamelCase``. 259 260Getters that never fail, and never return null, are named ``Foo()``, 261while all other getters use ``GetFoo()``. Getters can return an object 262value, via a ``Foo** aResult`` outparam (typical for an XPCOM getter), 263or as an ``already_AddRefed<Foo>`` (typical for a WebIDL getter, 264possibly with an ``ErrorResult& rv`` parameter), or occasionally as a 265``Foo*`` (typical for an internal getter for an object with a known 266lifetime). See `the bug 223255 <https://bugzilla.mozilla.org/show_bug.cgi?id=223255>`_ 267for more information. 268 269XPCOM getters always return primitive values via an outparam, while 270other getters normally use a return value. 271 272Method declarations must use, at most, one of the following keywords: 273``virtual``, ``override``, or ``final``. Use ``virtual`` to declare 274virtual methods, which do not override a base class method with the same 275signature. Use ``override`` to declare virtual methods which do 276override a base class method, with the same signature, but can be 277further overridden in derived classes. Use ``final`` to declare virtual 278methods which do override a base class method, with the same signature, 279but can NOT be further overridden in the derived classes. This should 280help the person reading the code fully understand what the declaration 281is doing, without needing to further examine base classes. 282 283.. note:: 284 285 For the rule on ``virtual/override/final``, clang-tidy provides the 286 ``modernize-use-override`` check with autofixes. 287 288 289Operators 290~~~~~~~~~ 291 292The unary keyword operator ``sizeof``, should have its operand parenthesized 293even if it is an expression; e.g. ``int8_t arr[64]; memset(arr, 42, sizeof(arr));``. 294 295 296Literals 297~~~~~~~~ 298 299Use ``\uXXXX`` unicode escapes for non-ASCII characters. The character 300set for XUL, DTD, script, and properties files is UTF-8, which is not easily 301readable. 302 303 304Prefixes 305~~~~~~~~ 306 307Follow these naming prefix conventions: 308 309 310Variable prefixes 311^^^^^^^^^^^^^^^^^ 312 313- k=constant (e.g. ``kNC_child``). Not all code uses this style; some 314 uses ``ALL_CAPS`` for constants. 315- g=global (e.g. ``gPrefService``) 316- a=argument (e.g. ``aCount``) 317- C++ Specific Prefixes 318 319 - s=static member (e.g. ``sPrefChecked``) 320 - m=member (e.g. ``mLength``) 321 - e=enum variants (e.g. ``enum Foo { eBar, eBaz }``). Enum classes 322 should use ``CamelCase`` instead (e.g. 323 ``enum class Foo { Bar, Baz }``). 324 325 326Global functions/macros/etc 327^^^^^^^^^^^^^^^^^^^^^^^^^^^ 328 329- Macros begin with ``MOZ_``, and are all caps (e.g. 330 ``MOZ_WOW_GOODNESS``). Note that older code uses the ``NS_`` prefix; 331 while these aren't being changed, you should only use ``MOZ_`` for 332 new macros. The only exception is if you're creating a new macro, 333 which is part of a set of related macros still using the old ``NS_`` 334 prefix. Then you should be consistent with the existing macros. 335 336 337Error Variables 338^^^^^^^^^^^^^^^ 339 340- Local variables that are assigned ``nsresult`` result codes should be named ``rv`` 341 (i.e., e.g., not ``res``, not ``result``, not ``foo``). `rv` should not be 342 used for bool or other result types. 343- Local variables that are assigned ``bool`` result codes should be named `ok`. 344 345 346C/C++ practices 347--------------- 348 349- **Have you checked for compiler warnings?** Warnings often point to 350 real bugs. `Many of them <https://searchfox.org/mozilla-central/source/build/moz.configure/warnings.configure>`__ 351 are enabled by default in the build system. 352- In C++ code, use ``nullptr`` for pointers. In C code, using ``NULL`` 353 or ``0`` is allowed. 354 355.. note:: 356 357 For the C++ rule, clang-tidy provides the ``modernize-use-nullptr`` check 358 with autofixes. 359 360- Don't use ``PRBool`` and ``PRPackedBool`` in C++, use ``bool`` 361 instead. 362- For checking if a ``std`` container has no items, don't use 363 ``size()``, instead use ``empty()``. 364- When testing a pointer, use ``(!myPtr)`` or ``(myPtr)``; 365 don't use ``myPtr != nullptr`` or ``myPtr == nullptr``. 366- Do not compare ``x == true`` or ``x == false``. Use ``(x)`` or 367 ``(!x)`` instead. ``if (x == true)`` may have semantics different from 368 ``if (x)``! 369 370.. note:: 371 372 clang-tidy provides the ``readability-simplify-boolean-expr`` check 373 with autofixes that checks for these and some other boolean expressions 374 that can be simplified. 375 376- In general, initialize variables with ``nsFoo aFoo = bFoo,`` and not 377 ``nsFoo aFoo(bFoo)``. 378 379 - For constructors, initialize member variables with : ``nsFoo 380 aFoo(bFoo)`` syntax. 381 382- To avoid warnings created by variables used only in debug builds, use 383 the 384 `DebugOnly<T> <https://developer.mozilla.org/docs/Mozilla/Debugging/DebugOnly%3CT%3E>`__ 385 helper when declaring them. 386- You should `use the static preference 387 API <https://developer.mozilla.org/docs/Mozilla/Preferences/Using_preferences_from_application_code>`__ for 388 working with preferences. 389- One-argument constructors, that are not copy or move constructors, 390 should generally be marked explicit. Exceptions should be annotated 391 with ``MOZ_IMPLICIT``. 392- Use ``char32_t`` as the return type or argument type of a method that 393 returns or takes as argument a single Unicode scalar value. (Don't 394 use UTF-32 strings, though.) 395- Prefer unsigned types for semantically-non-negative integer values. 396- When operating on integers that could overflow, use ``CheckedInt``. 397- Avoid the usage of ``typedef``, instead, please use ``using`` instead. 398 399.. note:: 400 401 For parts of this rule, clang-tidy provides the ``modernize-use-using`` 402 check with autofixes. 403 404 405Header files 406------------ 407 408Since the Firefox code base is huge and uses a monolithic build, it is 409of utmost importance for keeping build times reasonable to limit the 410number of included files in each translation unit to the required minimum. 411Exported header files need particular attention in this regard, since their 412included files propagate, and many of them are directly or indirectly 413included in a large number of translation units. 414 415- Include guards are named per the Google coding style (i.e. upper snake 416 case with a single trailing underscore). They should not include a 417 leading ``MOZ_`` or ``MOZILLA_``. For example, ``dom/media/foo.h`` 418 would use the guard ``DOM_MEDIA_FOO_H_``. 419- Forward-declare classes in your header files, instead of including 420 them, whenever possible. For example, if you have an interface with a 421 ``void DoSomething(nsIContent* aContent)`` function, forward-declare 422 with ``class nsIContent;`` instead of ``#include "nsIContent.h"``. 423 If a "forwarding header" is provided for a type, include that instead of 424 putting the literal forward declaration(s) in your header file. E.g. for 425 some JavaScript types, there is ``js/TypeDecls.h``, for the string types 426 there is ``StringFwd.h``. One reason for this is that this allows 427 changing a type to a type alias by only changing the forwarding header. 428 The following uses of a type can be done with a forward declaration only: 429 430 - Parameter or return type in a function declaration 431 - Member/local variable pointer or reference type 432 - Use as a template argument (not in all cases) in a member/local variable type 433 - Defining a type alias 434 435 The following uses of a type require a full definition: 436 437 - Base class 438 - Member/local variable type 439 - Use with delete or new 440 - Use as a template argument (not in all cases) 441 - Any uses of non-scoped enum types 442 - Enum values of a scoped enum type 443 444 Use as a template argument is somewhat tricky. It depends on how the 445 template uses the type. E.g. ``mozilla::Maybe<T>`` and ``AutoTArray<T>`` 446 always require a full definition of ``T`` because the size of the 447 template instance depends on the size of ``T``. ``RefPtr<T>`` and 448 ``UniquePtr<T>`` don't require a full definition (because their 449 pointer member always has the same size), but their destructor 450 requires a full definition. If you encounter a template that cannot 451 be instantiated with a forward declaration only, but it seems 452 it should be possible, please file a bug (if it doesn't exist yet). 453 454 Therefore, also consider the following guidelines to allow using forward 455 declarations as widely as possible. 456- Inline function bodies in header files often pull in a lot of additional 457 dependencies. Be mindful when adding or extending inline function bodies, 458 and consider moving the function body to the cpp file or to a separate 459 header file that is not included everywhere. Bug 1677553 intends to provide 460 a more specific guideline on this. 461- Consider the use of the `Pimpl idiom <https://en.cppreference.com/w/cpp/language/pimpl>`__, 462 i.e. hide the actual implementation in a separate ``Impl`` class that is 463 defined in the implementation file and only expose a ``class Impl;`` forward 464 declaration and ``UniquePtr<Impl>`` member in the header file. 465- Do not use non-scoped enum types. These cannot be forward-declared. Use 466 scoped enum types instead, and forward declare them when possible. 467- Avoid nested types that need to be referenced from outside the class. 468 These cannot be forward declared. Place them in a namespace instead, maybe 469 in an extra inner namespace, and forward declare them where possible. 470- Avoid mixing declarations with different sets of dependencies in a single 471 header file. This is generally advisable, but even more so when some of these 472 declarations are used by a subset of the translation units that include the 473 combined header file only. Consider such a badly mixed header file like: 474 475 .. code-block:: cpp 476 477 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ 478 /* vim: set ts=8 sts=2 et sw=2 tw=80: */ 479 /* This Source Code Form is subject to the terms of the Mozilla Public 480 * License, v. 2.0. If a copy of the MPL was not distributed with this file, 481 * You can obtain one at http://mozilla.org/MPL/2.0/. */ 482 483 #ifndef BAD_MIXED_FILE_H_ 484 #define BAD_MIXED_FILE_H_ 485 486 // Only this include is needed for the function declaration below. 487 #include "nsCOMPtr.h" 488 489 // These includes are only needed for the class definition. 490 #include "nsIFile.h" 491 #include "mozilla/ComplexBaseClass.h" 492 493 namespace mozilla { 494 495 class WrappedFile : public nsIFile, ComplexBaseClass { 496 // ... class definition left out for clarity 497 }; 498 499 // Assuming that most translation units that include this file only call 500 // the function, but don't need the class definition, this should be in a 501 // header file on its own in order to avoid pulling in the other 502 // dependencies everywhere. 503 nsCOMPtr<nsIFile> CreateDefaultWrappedFile(nsCOMPtr<nsIFile>&& aFileToWrap); 504 505 } // namespace mozilla 506 507 #endif // BAD_MIXED_FILE_H_ 508 509 510An example header file based on these rules (with some extra comments): 511 512.. code-block:: cpp 513 514 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ 515 /* vim: set ts=8 sts=2 et sw=2 tw=80: */ 516 /* This Source Code Form is subject to the terms of the Mozilla Public 517 * License, v. 2.0. If a copy of the MPL was not distributed with this file, 518 * You can obtain one at http://mozilla.org/MPL/2.0/. */ 519 520 #ifndef DOM_BASE_FOO_H_ 521 #define DOM_BASE_FOO_H_ 522 523 // Include guards should come at the very beginning and always use exactly 524 // the style above. Otherwise, compiler optimizations that avoid rescanning 525 // repeatedly included headers might not hit and cause excessive compile 526 // times. 527 528 #include <cstdint> 529 #include "nsCOMPtr.h" // This is needed because we have a nsCOMPtr<T> data member. 530 531 class nsIFile; // Used as a template argument only. 532 enum class nsresult : uint32_t; // Used as a parameter type only. 533 template <class T> 534 class RefPtr; // Used as a return type only. 535 536 namespace mozilla::dom { 537 538 class Document; // Used as a template argument only. 539 540 // Scoped enum, not as a nested type, so it can be 541 // forward-declared elsewhere. 542 enum class FooKind { Small, Big }; 543 544 class Foo { 545 public: 546 // Do not put the implementation in the header file, it would 547 // require including nsIFile.h 548 Foo(nsCOMPtr<nsIFile> aFile, FooKind aFooKind); 549 550 RefPtr<Document> CreateDocument(); 551 552 void SetResult(nsresult aResult); 553 554 // Even though we will default this destructor, do this in the 555 // implementation file since we would otherwise need to include 556 // nsIFile.h in the header. 557 ~Foo(); 558 559 private: 560 nsCOMPtr<nsIFile> mFile; 561 }; 562 563 } // namespace mozilla::dom 564 565 #endif // DOM_BASE_FOO_H_ 566 567 568Corresponding implementation file: 569 570.. code-block:: cpp 571 572 /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ 573 /* vim: set ts=8 sts=2 et sw=2 tw=80: */ 574 /* This Source Code Form is subject to the terms of the Mozilla Public 575 * License, v. 2.0. If a copy of the MPL was not distributed with this file, 576 * You can obtain one at http://mozilla.org/MPL/2.0/. */ 577 578 #include "mozilla/dom/Foo.h" // corresponding header 579 580 #include "mozilla/Assertions.h" // Needed for MOZ_ASSERT. 581 #include "mozilla/dom/Document.h" // Needed because we construct a Document. 582 #include "nsError.h" // Needed because we use NS_OK aka nsresult::NS_OK. 583 #include "nsIFile.h" // This is needed because our destructor indirectly calls delete nsIFile in a template instance. 584 585 namespace mozilla::dom { 586 587 // Do not put the implementation in the header file, it would 588 // require including nsIFile.h 589 Foo::Foo(nsCOMPtr<nsIFile> aFile, FooKind aFooKind) 590 : mFile{std::move(aFile)} { 591 } 592 593 RefPtr<Document> Foo::CreateDocument() { 594 return MakeRefPtr<Document>(); 595 } 596 597 void Foo::SetResult(nsresult aResult) { 598 MOZ_ASSERT(aResult != NS_OK); 599 600 // do something with aResult 601 } 602 603 // Even though we default this destructor, do this in the 604 // implementation file since we would otherwise need to include 605 // nsIFile.h in the header. 606 Foo::~Foo() = default; 607 608 } // namespace mozilla::dom 609 610 611Include directives 612------------------ 613 614- Ordering: 615 616 - In an implementation file (cpp file), the very first include directive 617 should include the corresponding header file, followed by a blank line. 618 - Any conditional includes (depending on some ``#ifdef`` or similar) follow 619 after non-conditional includes. Don't mix them in. 620 - Don't place comments between non-conditional includes. 621 622 Bug 1679522 addresses automating the ordering via clang-format, which 623 is going to enforce some stricter rules. Expect the includes to be reordered. 624 If you include third-party headers that are not self-contained, and therefore 625 need to be included in a particular order, enclose those (and only those) 626 between ``// clang-format off`` and ``// clang-format on``. This should not be 627 done for Mozilla headers, which should rather be made self-contained if they 628 are not. 629 630- Brackets vs. quotes: C/C++ standard library headers are included using 631 brackets (e.g. ``#include <cstdint>``), all other include directives use 632 (double) quotes (e.g. ``#include "mozilla/dom/Document.h``). 633- Exported headers should always be included from their exported path, not 634 from their source path in the tree, even if available locally. E.g. always 635 do ``#include "mozilla/Vector.h"``, not ``#include "Vector.h"``, even 636 from within `mfbt`. 637- Generally, you should include exactly those headers that are needed, not 638 more and not less. Unfortunately this is not easy to see. Maybe C++20 639 modules will bring improvements to this, but it will take a long time 640 to be adopted. 641- The basic rule is that if you literally use a symbol in your file that 642 is declared in a header A.h, include that header. In particular in header 643 files, check if a forward declaration or including a forwarding header is 644 sufficient, see section :ref:`Header files`. 645 646 There are cases where this basic rule is not sufficient. Some cases where 647 you need to include additional headers are: 648 649 - You reference a member of a type that is not literally mentioned in your 650 code, but, e.g. is the return type of a function you are calling. 651 652 There are also cases where the basic rule leads to redundant includes. Note 653 that "redundant" here does not refer to "accidentally redundant" headers, 654 e.g. at the time of writing ``mozilla/dom/BodyUtil.h`` includes 655 ``mozilla/dom/FormData.h``, but it doesn't need to (it only needs a forward 656 declaration), so including ``mozilla/dom/FormData.h`` is "accidentally 657 redundant" when including ``mozilla/dom/BodyUtil.h``. The includes of 658 ``mozilla/dom/BodyUtil.h`` might change at any time, so if a file that 659 includes ``mozilla/dom/BodyUtil.h`` needs a full definition of 660 ``mozilla::dom::FormData``, it should includes ``mozilla/dom/FormData.h`` 661 itself. In fact, these "accidentally redundant" headers MUST be included. 662 Relying on accidentally redundant includes makes any change to a header 663 file extremely hard, in particular when considering that the set of 664 accidentally redundant includes differs between platforms. 665 But some cases in fact are non-accidentally redundant, and these can and 666 typically should not be repeated: 667 668 - The includes of the header file do not need to be repeated in its 669 corresponding implementation file. Rationale: the implementation file and 670 its corresponding header file are tightly coupled per se. 671 672 Macros are a special case. Generally, the literal rule also applies here, 673 i.e. if the macro definition references a symbol, the file containing the 674 macro definition should include the header defining the symbol. E.g. 675 ``NS_IMPL_CYCLE_COLLECTING_NATIVE_RELEASE`` defined in ``nsISupportsImpl.h`` 676 makes use of ``MOZ_ASSERT`` defined in ``mozilla/Assertions.h``, so 677 ``nsISupportsImpl.h`` includes ``mozilla/Assertions.h``. However, this 678 requires human judgment of what is intended, since technically only the 679 invocations of the macro reference a symbol (and that's how 680 include-what-you-use handles this). It might depend on the 681 context or parameters which symbol is actually referenced, and sometimes 682 this is on purpose. In these cases, the user of the macro needs to include 683 the required header(s). 684 685 686 687COM and pointers 688---------------- 689 690- Use ``nsCOMPtr<>`` 691 If you don't know how to use it, start looking in the code for 692 examples. The general rule, is that the very act of typing 693 ``NS_RELEASE`` should be a signal to you to question your code: 694 "Should I be using ``nsCOMPtr`` here?". Generally the only valid use 695 of ``NS_RELEASE`` is when you are storing refcounted pointers in a 696 long-lived datastructure. 697- Declare new XPCOM interfaces using `XPIDL <https://developer.mozilla.org/docs/Mozilla/Tech/XPIDL>`__, so they 698 will be scriptable. 699- Use `nsCOMPtr <https://developer.mozilla.org/docs/Mozilla/Tech/XPCOM/Reference/Glue_classes/nsCOMPtr>`__ for strong references, and 700 `nsWeakPtr <https://developer.mozilla.org/docs/Mozilla/Tech/XPCOM/Weak_reference>`__ for weak references. 701- Don't use ``QueryInterface`` directly. Use ``CallQueryInterface`` or 702 ``do_QueryInterface`` instead. 703- Use `Contract 704 IDs <news://news.mozilla.org/3994AE3E.D96EF810@netscape.com>`__, 705 instead of CIDs with ``do_CreateInstance``/``do_GetService``. 706- Use pointers, instead of references for function out parameters, even 707 for primitive types. 708 709 710IDL 711--- 712 713 714Use leading-lowercase, or "interCaps" 715~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 716 717When defining a method or attribute in IDL, the first letter should be 718lowercase, and each following word should be capitalized. For example: 719 720.. code-block:: cpp 721 722 long updateStatusBar(); 723 724 725Use attributes wherever possible 726~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 727 728Whenever you are retrieving or setting a single value, without any 729context, you should use attributes. Don't use two methods when you could 730use an attribute. Using attributes logically connects the getting and 731setting of a value, and makes scripted code look cleaner. 732 733This example has too many methods: 734 735.. code-block:: cpp 736 737 interface nsIFoo : nsISupports 738 { 739 long getLength(); 740 void setLength(in long length); 741 long getColor(); 742 }; 743 744The code below will generate the exact same C++ signature, but is more 745script-friendly. 746 747.. code-block:: cpp 748 749 interface nsIFoo : nsISupports 750 { 751 attribute long length; 752 readonly attribute long color; 753 }; 754 755 756Use Java-style constants 757~~~~~~~~~~~~~~~~~~~~~~~~ 758 759When defining scriptable constants in IDL, the name should be all 760uppercase, with underscores between words: 761 762.. code-block:: cpp 763 764 const long ERROR_UNDEFINED_VARIABLE = 1; 765 766 767See also 768~~~~~~~~ 769 770For details on interface development, as well as more detailed style 771guides, see the `Interface development 772guide <https://developer.mozilla.org/docs/Mozilla/Developer_guide/Interface_development_guide>`__. 773 774 775Error handling 776-------------- 777 778 779Check for errors early and often 780~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 781 782Every time you make a call into an XPCOM function, you should check for 783an error condition. You need to do this even if you know that call will 784never fail. Why? 785 786- Someone may change the callee in the future to return a failure 787 condition. 788- The object in question may live on another thread, another process, 789 or possibly even another machine. The proxy could have failed to make 790 your call in the first place. 791 792Also, when you make a new function which is failable (i.e. it will 793return a ``nsresult`` or a ``bool`` that may indicate an error), you should 794explicitly mark the return value should always be checked. For example: 795 796:: 797 798 // for IDL. 799 [must_use] nsISupports 800 create(); 801 802 // for C++, add this in *declaration*, do not add it again in implementation. 803 [[nodiscard]] nsresult 804 DoSomething(); 805 806There are some exceptions: 807 808- Predicates or getters, which return ``bool`` or ``nsresult``. 809- IPC method implementation (For example, ``bool RecvSomeMessage()``). 810- Most callers will check the output parameter, see below. 811 812.. code-block:: cpp 813 814 nsresult 815 SomeMap::GetValue(const nsString& key, nsString& value); 816 817If most callers need to check the output value first, then adding 818``[[nodiscard]]`` might be too verbose. In this case, change the return value 819to void might be a reasonable choice. 820 821There is also a static analysis attribute ``[[nodiscard]]``, which can 822be added to class declarations, to ensure that those declarations are 823always used when they are returned. 824 825 826Use the NS_WARN_IF macro when errors are unexpected. 827~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 828 829The ``NS_WARN_IF`` macro can be used to issue a console warning, in debug 830builds if the condition fails. This should only be used when the failure 831is unexpected and cannot be caused by normal web content. 832 833If you are writing code which wants to issue warnings when methods fail, 834please either use ``NS_WARNING`` directly, or use the new ``NS_WARN_IF`` macro. 835 836.. code-block:: cpp 837 838 if (NS_WARN_IF(somethingthatshouldbefalse)) { 839 return NS_ERROR_INVALID_ARG; 840 } 841 842 if (NS_WARN_IF(NS_FAILED(rv))) { 843 return rv; 844 } 845 846Previously, the ``NS_ENSURE_*`` macros were used for this purpose, but 847those macros hide return statements, and should not be used in new code. 848(This coding style rule isn't generally agreed, so use of ``NS_ENSURE_*`` 849can be valid.) 850 851 852Return from errors immediately 853~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 854 855In most cases, your knee-jerk reaction should be to return from the 856current function, when an error condition occurs. Don't do this: 857 858.. code-block:: cpp 859 860 rv = foo->Call1(); 861 if (NS_SUCCEEDED(rv)) { 862 rv = foo->Call2(); 863 if (NS_SUCCEEDED(rv)) { 864 rv = foo->Call3(); 865 } 866 } 867 return rv; 868 869Instead, do this: 870 871.. code-block:: cpp 872 873 rv = foo->Call1(); 874 if (NS_FAILED(rv)) { 875 return rv; 876 } 877 878 rv = foo->Call2(); 879 if (NS_FAILED(rv)) { 880 return rv; 881 } 882 883 rv = foo->Call3(); 884 if (NS_FAILED(rv)) { 885 return rv; 886 } 887 888Why? Error handling should not obfuscate the logic of the code. The 889author's intent, in the first example, was to make 3 calls in 890succession. Wrapping the calls in nested if() statements, instead 891obscured the most likely behavior of the code. 892 893Consider a more complicated example to hide a bug: 894 895.. code-block:: cpp 896 897 bool val; 898 rv = foo->GetBooleanValue(&val); 899 if (NS_SUCCEEDED(rv) && val) { 900 foo->Call1(); 901 } else { 902 foo->Call2(); 903 } 904 905The intent of the author, may have been, that ``foo->Call2()`` would only 906happen when val had a false value. In fact, ``foo->Call2()`` will also be 907called, when ``foo->GetBooleanValue(&val)`` fails. This may, or may not, 908have been the author's intent. It is not clear from this code. Here is 909an updated version: 910 911.. code-block:: cpp 912 913 bool val; 914 rv = foo->GetBooleanValue(&val); 915 if (NS_FAILED(rv)) { 916 return rv; 917 } 918 if (val) { 919 foo->Call1(); 920 } else { 921 foo->Call2(); 922 } 923 924In this example, the author's intent is clear, and an error condition 925avoids both calls to ``foo->Call1()`` and ``foo->Call2();`` 926 927*Possible exceptions:* Sometimes it is not fatal if a call fails. For 928instance, if you are notifying a series of observers that an event has 929fired, it might be trivial that one of these notifications failed: 930 931.. code-block:: cpp 932 933 for (size_t i = 0; i < length; ++i) { 934 // we don't care if any individual observer fails 935 observers[i]->Observe(foo, bar, baz); 936 } 937 938Another possibility, is you are not sure if a component exists or is 939installed, and you wish to continue normally, if the component is not 940found. 941 942.. code-block:: cpp 943 944 nsCOMPtr<nsIMyService> service = do_CreateInstance(NS_MYSERVICE_CID, &rv); 945 // if the service is installed, then we'll use it. 946 if (NS_SUCCEEDED(rv)) { 947 // non-fatal if this fails too, ignore this error. 948 service->DoSomething(); 949 950 // this is important, handle this error! 951 rv = service->DoSomethingImportant(); 952 if (NS_FAILED(rv)) { 953 return rv; 954 } 955 } 956 957 // continue normally whether or not the service exists. 958 959 960Strings 961------- 962 963.. note:: 964 965 This section overlaps with the more verbose advice given in 966 `Internal strings <https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Guide/Internal_strings>`__. 967 These should eventually be merged. For now, please refer to that guide for 968 more advice. 969 970- String arguments to functions should be declared as ``[const] nsA[C]String&``. 971- Prefer using string literals. In particular, use empty string literals, 972 i.e. ``u""_ns`` or ``""_ns``, instead of ``Empty[C]String()`` or 973 ``const nsAuto[C]String empty;``. Use ``Empty[C]String()`` only if you 974 specifically need a ``const ns[C]String&``, e.g. with the ternary operator 975 or when you need to return/bind to a reference or take the address of the 976 empty string. 977- For 16-bit literal strings, use ``u"..."_ns`` or, if necessary 978 ``NS_LITERAL_STRING_FROM_CSTRING(...)`` instead of ``nsAutoString()`` 979 or other ways that would do a run-time conversion. 980 See :ref:`Avoid runtime conversion of string literals <Avoid runtime conversion of string literals>` below. 981- To compare a string with a literal, use ``.EqualsLiteral("...")``. 982- Use ``str.IsEmpty()`` instead of ``str.Length() == 0``. 983- Use ``str.Truncate()`` instead of ``str.SetLength(0)``, 984 ``str.Assign(""_ns)`` or ``str.AssignLiteral("")``. 985- Don't use functions from ``ctype.h`` (``isdigit()``, ``isalpha()``, 986 etc.) or from ``strings.h`` (``strcasecmp()``, ``strncasecmp()``). 987 These are locale-sensitive, which makes them inappropriate for 988 processing protocol text. At the same time, they are too limited to 989 work properly for processing natural-language text. Use the 990 alternatives in ``mozilla/TextUtils.h`` and in ``nsUnicharUtils.h`` 991 in place of ``ctype.h``. In place of ``strings.h``, prefer the 992 ``nsStringComparator`` facilities for comparing strings or if you 993 have to work with zero-terminated strings, use ``nsCRT.h`` for 994 ASCII-case-insensitive comparison. 995 996 997Use the ``Auto`` form of strings for local values 998~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 999 1000When declaring a local, short-lived ``nsString`` class, always use 1001``nsAutoString`` or ``nsAutoCString``. These pre-allocate a 64-byte 1002buffer on the stack, and avoid fragmenting the heap. Don't do this: 1003 1004.. code-block:: cpp 1005 1006 nsresult 1007 foo() 1008 { 1009 nsCString bar; 1010 .. 1011 } 1012 1013instead: 1014 1015.. code-block:: cpp 1016 1017 nsresult 1018 foo() 1019 { 1020 nsAutoCString bar; 1021 .. 1022 } 1023 1024 1025Be wary of leaking values from non-XPCOM functions that return char\* or PRUnichar\* 1026~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1027 1028It is an easy trap to return an allocated string, from an internal 1029helper function, and then using that function inline in your code, 1030without freeing the value. Consider this code: 1031 1032.. code-block:: cpp 1033 1034 static char* 1035 GetStringValue() 1036 { 1037 .. 1038 return resultString.ToNewCString(); 1039 } 1040 1041 .. 1042 WarnUser(GetStringValue()); 1043 1044In the above example, ``WarnUser`` will get the string allocated from 1045``resultString.ToNewCString()`` and throw away the pointer. The 1046resulting value is never freed. Instead, either use the string classes, 1047to make sure your string is automatically freed when it goes out of 1048scope, or make sure that your string is freed. 1049 1050Automatic cleanup: 1051 1052.. code-block:: cpp 1053 1054 static void 1055 GetStringValue(nsAWritableCString& aResult) 1056 { 1057 .. 1058 aResult.Assign("resulting string"); 1059 } 1060 1061 .. 1062 nsAutoCString warning; 1063 GetStringValue(warning); 1064 WarnUser(warning.get()); 1065 1066Free the string manually: 1067 1068.. code-block:: cpp 1069 1070 static char* 1071 GetStringValue() 1072 { 1073 .. 1074 return resultString.ToNewCString(); 1075 } 1076 1077 .. 1078 char* warning = GetStringValue(); 1079 WarnUser(warning); 1080 nsMemory::Free(warning); 1081 1082.. _Avoid runtime conversion of string literals: 1083 1084Avoid runtime conversion of string literals 1085~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1086 1087It is very common to need to assign the value of a literal string, such 1088as ``"Some String"``, into a unicode buffer. Instead of using ``nsString``'s 1089``AssignLiteral`` and ``AppendLiteral``, use a user-defined literal like `u"foo"_ns` 1090instead. On most platforms, this will force the compiler to compile in a 1091raw unicode string, and assign it directly. In cases where the literal is defined 1092via a macro that is used in both 8-bit and 16-bit ways, you can use 1093`NS_LITERAL_STRING_FROM_CSTRING` to do the conversion at compile time. 1094 1095Incorrect: 1096 1097.. code-block:: cpp 1098 1099 nsAutoString warning; 1100 warning.AssignLiteral("danger will robinson!"); 1101 ... 1102 foo->SetStringValue(warning); 1103 ... 1104 bar->SetUnicodeValue(warning.get()); 1105 1106Correct: 1107 1108.. code-block:: cpp 1109 1110 constexpr auto warning = u"danger will robinson!"_ns; 1111 ... 1112 // if you'll be using the 'warning' string, you can still use it as before: 1113 foo->SetStringValue(warning); 1114 ... 1115 bar->SetUnicodeValue(warning.get()); 1116 1117 // alternatively, use the wide string directly: 1118 foo->SetStringValue(u"danger will robinson!"_ns); 1119 ... 1120 1121 // if a macro is the source of a 8-bit literal and you cannot change it, use 1122 // NS_LITERAL_STRING_FROM_CSTRING, but only if necessary. 1123 #define MY_MACRO_LITERAL "danger will robinson!" 1124 foo->SetStringValue(NS_LITERAL_STRING_FROM_CSTRING(MY_MACRO_LITERAL)); 1125 1126 // If you need to pass to a raw const char16_t *, there's no benefit to 1127 // go through our string classes at all, just do... 1128 bar->SetUnicodeValue(u"danger will robinson!"); 1129 1130 // .. or, again, if a macro is the source of a 8-bit literal 1131 bar->SetUnicodeValue(u"" MY_MACRO_LITERAL); 1132 1133 1134Usage of PR_(MAX|MIN|ABS|ROUNDUP) macro calls 1135--------------------------------------------- 1136 1137Use the standard-library functions (``std::max``), instead of 1138``PR_(MAX|MIN|ABS|ROUNDUP)``. 1139 1140Use ``mozilla::Abs`` instead of ``PR_ABS``. All ``PR_ABS`` calls in C++ code have 1141been replaced with ``mozilla::Abs`` calls, in `bug 1142847480 <https://bugzilla.mozilla.org/show_bug.cgi?id=847480>`__. All new 1143code in ``Firefox/core/toolkit`` needs to ``#include "nsAlgorithm.h"`` and 1144use the ``NS_foo`` variants instead of ``PR_foo``, or 1145``#include "mozilla/MathAlgorithms.h"`` for ``mozilla::Abs``. 1146