diff options
Diffstat (limited to '')
| -rw-r--r-- | docs/code-quality/coding-style/coding_style_cpp.rst | 1150 |
1 files changed, 1150 insertions, 0 deletions
diff --git a/docs/code-quality/coding-style/coding_style_cpp.rst b/docs/code-quality/coding-style/coding_style_cpp.rst new file mode 100644 index 0000000000..cb4764cea5 --- /dev/null +++ b/docs/code-quality/coding-style/coding_style_cpp.rst @@ -0,0 +1,1150 @@ +================ +C++ Coding style +================ + + +This document attempts to explain the basic styles and patterns used in +the Mozilla codebase. New code should try to conform to these standards, +so it is as easy to maintain as existing code. There are exceptions, but +it's still important to know the rules! + +This article is particularly for those new to the Mozilla codebase, and +in the process of getting their code reviewed. Before requesting a +review, please read over this document, making sure that your code +conforms to recommendations. + +.. container:: blockIndicator warning + + The Firefox code base adopts parts of the `Google Coding style for C++ + code <https://google.github.io/styleguide/cppguide.html>`__, but not all of its rules. + A few rules are followed across the code base, others are intended to be + followed in new or significantly revised code. We may extend this list in the + future, when we evaluate the Google Coding Style for C++ Code further and/or update + our coding practices. However, the plan is not to adopt all rules of the Google Coding + Style for C++ Code. Some rules are explicitly unlikely to be adopted at any time. + + Followed across the code base: + + - `Formatting <https://google.github.io/styleguide/cppguide.html#Formatting>`__, + except for subsections noted here otherwise + - `Implicit Conversions <https://google.github.io/styleguide/cppguide.html#Implicit_Conversions>`__, + which is enforced by a custom clang-plugin check, unless explicitly overridden using + ``MOZ_IMPLICIT`` + + Followed in new/significantly revised code: + + - `Include guards <https://google.github.io/styleguide/cppguide.html#The__define_Guard>`__ + + Unlikely to be ever adopted: + + - `Forward declarations <https://google.github.io/styleguide/cppguide.html#Forward_Declarations>`__ + - `Formatting/Conditionals <https://google.github.io/styleguide/cppguide.html#Conditionals>`__ + w.r.t. curly braces around inner statements, we require them in all cases where the + Google style allows to leave them out for single-line conditional statements + + This list reflects the state of the Google Google Coding Style for C++ Code as of + 2020-07-17. It may become invalid when the Google modifies its Coding Style. + + +Formatting code +--------------- + +Formatting is done automatically via clang-format, and controlled via in-tree +configuration files. See :ref:`Formatting C++ Code With clang-format` +for more information. + +Unix-style linebreaks (``\n``), not Windows-style (``\r\n``). You can +convert patches, with DOS newlines to Unix via the ``dos2unix`` utility, +or your favorite text editor. + +Static analysis +--------------- + +Several of the rules in the Google C++ coding styles and the additions mentioned below +can be checked via clang-tidy (some rules are from the upstream clang-tidy, some are +provided via a mozilla-specific plugin). Some of these checks also allow fixes to +be automatically applied. + +``mach static-analysis`` provides a convenient way to run these checks. For example, +for the check called ``google-readability-braces-around-statements``, you can run: + +.. code-block:: shell + + ./mach static-analysis check --checks="-*,google-readability-braces-around-statements" --fix <file> + +It may be necessary to reformat the files after automatically applying fixes, see +:ref:`Formatting C++ Code With clang-format`. + +Additional rules +---------------- + +*The norms in this section should be followed for new code. For existing code, +use the prevailing style in a file or module, ask the owner if you are +in another team's codebase or it's not clear what style to use.* + + + + +Control structures +~~~~~~~~~~~~~~~~~~ + +Always brace controlled statements, even a single-line consequent of +``if else else``. This is redundant, typically, but it avoids dangling +else bugs, so it's safer at scale than fine-tuning. + +Examples: + +.. code-block:: cpp + + if (...) { + } else if (...) { + } else { + } + + while (...) { + } + + do { + } while (...); + + for (...; ...; ...) { + } + + switch (...) { + case 1: { + // When you need to declare a variable in a switch, put the block in braces. + int var; + break; + } + case 2: + ... + break; + default: + break; + } + +``else`` should only ever be followed by ``{`` or ``if``; i.e., other +control keywords are not allowed and should be placed inside braces. + +.. note:: + + For this rule, clang-tidy provides the ``google-readability-braces-around-statements`` + check with autofixes. + + +C++ namespaces +~~~~~~~~~~~~~~ + +Mozilla project C++ declarations should be in the ``mozilla`` +namespace. Modules should avoid adding nested namespaces under +``mozilla``, unless they are meant to contain names which have a high +probability of colliding with other names in the code base. For example, +``Point``, ``Path``, etc. Such symbols can be put under +module-specific namespaces, under ``mozilla``, with short +all-lowercase names. Other global namespaces besides ``mozilla`` are +not allowed. + +No ``using`` directives are allowed in header files, except inside class +definitions or functions. (We don't want to pollute the global scope of +compilation units that use the header file.) + +.. note:: + + For parts of this rule, clang-tidy provides the ``google-global-names-in-headers`` + check. It only detects ``using namespace`` directives in the global namespace. + + +``using namespace ...;`` is only allowed in ``.cpp`` files after all +``#include``\ s. Prefer to wrap code in ``namespace ... { ... };`` +instead, if possible. ``using namespace ...;``\ should always specify +the fully qualified namespace. That is, to use ``Foo::Bar`` do not +write ``using namespace Foo; using namespace Bar;``, write +``using namespace Foo::Bar;`` + +Use nested namespaces (ex: ``namespace mozilla::widget {`` + +.. note:: + + clang-tidy provides the ``modernize-concat-nested-namespaces`` + check with autofixes. + + +Anonymous namespaces +~~~~~~~~~~~~~~~~~~~~ + +We prefer using ``static``, instead of anonymous C++ namespaces. This may +change once there is better debugger support (especially on Windows) for +placing breakpoints, etc. on code in anonymous namespaces. You may still +use anonymous namespaces for things that can't be hidden with ``static``, +such as types, or certain objects which need to be passed to template +functions. + + +C++ classes +~~~~~~~~~~~~ + +.. code-block:: cpp + + namespace mozilla { + + class MyClass : public A + { + ... + }; + + class MyClass + : public X + , public Y + { + public: + MyClass(int aVar, int aVar2) + : mVar(aVar) + , mVar2(aVar2) + { + ... + } + + // Special member functions, like constructors, that have default bodies + // should use '= default' annotation instead. + MyClass() = default; + + // Unless it's a copy or move constructor or you have a specific reason to allow + // implicit conversions, mark all single-argument constructors explicit. + explicit MyClass(OtherClass aArg) + { + ... + } + + // This constructor can also take a single argument, so it also needs to be marked + // explicit. + explicit MyClass(OtherClass aArg, AnotherClass aArg2 = AnotherClass()) + { + ... + } + + int LargerFunction() + { + ... + ... + } + + private: + int mVar; + }; + + } // namespace mozilla + +Define classes using the style given above. + +.. note:: + + For the rule on ``= default``, clang-tidy provides the ``modernize-use-default`` + check with autofixes. + + For the rule on explicit constructors and conversion operators, clang-tidy + provides the ``mozilla-implicit-constructor`` check. + +Existing classes in the global namespace are named with a short prefix +(For example, ``ns``) as a pseudo-namespace. + + +Methods and functions +~~~~~~~~~~~~~~~~~~~~~ + + +C/C++ +^^^^^ + +In C/C++, method names should use ``UpperCamelCase``. + +Getters that never fail, and never return null, are named ``Foo()``, +while all other getters use ``GetFoo()``. Getters can return an object +value, via a ``Foo** aResult`` outparam (typical for an XPCOM getter), +or as an ``already_AddRefed<Foo>`` (typical for a WebIDL getter, +possibly with an ``ErrorResult& rv`` parameter), or occasionally as a +``Foo*`` (typical for an internal getter for an object with a known +lifetime). See `the bug 223255 <https://bugzilla.mozilla.org/show_bug.cgi?id=223255>`_ +for more information. + +XPCOM getters always return primitive values via an outparam, while +other getters normally use a return value. + +Method declarations must use, at most, one of the following keywords: +``virtual``, ``override``, or ``final``. Use ``virtual`` to declare +virtual methods, which do not override a base class method with the same +signature. Use ``override`` to declare virtual methods which do +override a base class method, with the same signature, but can be +further overridden in derived classes. Use ``final`` to declare virtual +methods which do override a base class method, with the same signature, +but can NOT be further overridden in the derived classes. This should +help the person reading the code fully understand what the declaration +is doing, without needing to further examine base classes. + +.. note:: + + For the rule on ``virtual/override/final``, clang-tidy provides the + ``modernize-use-override`` check with autofixes. + + +Operators +~~~~~~~~~ + +The unary keyword operator ``sizeof``, should have its operand parenthesized +even if it is an expression; e.g. ``int8_t arr[64]; memset(arr, 42, sizeof(arr));``. + + +Literals +~~~~~~~~ + +Use ``\uXXXX`` unicode escapes for non-ASCII characters. The character +set for XUL, script, and properties files is UTF-8, which is not easily +readable. + + +Prefixes +~~~~~~~~ + +Follow these naming prefix conventions: + + +Variable prefixes +^^^^^^^^^^^^^^^^^ + +- k=constant (e.g. ``kNC_child``). Not all code uses this style; some + uses ``ALL_CAPS`` for constants. +- g=global (e.g. ``gPrefService``) +- a=argument (e.g. ``aCount``) +- C++ Specific Prefixes + + - s=static member (e.g. ``sPrefChecked``) + - m=member (e.g. ``mLength``) + - e=enum variants (e.g. ``enum Foo { eBar, eBaz }``). Enum classes + should use ``CamelCase`` instead (e.g. + ``enum class Foo { Bar, Baz }``). + + +Global functions/macros/etc +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +- Macros begin with ``MOZ_``, and are all caps (e.g. + ``MOZ_WOW_GOODNESS``). Note that older code uses the ``NS_`` prefix; + while these aren't being changed, you should only use ``MOZ_`` for + new macros. The only exception is if you're creating a new macro, + which is part of a set of related macros still using the old ``NS_`` + prefix. Then you should be consistent with the existing macros. + + +Error Variables +^^^^^^^^^^^^^^^ + +- Local variables that are assigned ``nsresult`` result codes should be named ``rv`` + (i.e., e.g., not ``res``, not ``result``, not ``foo``). `rv` should not be + used for bool or other result types. +- Local variables that are assigned ``bool`` result codes should be named `ok`. + + +C/C++ practices +--------------- + +- **Have you checked for compiler warnings?** Warnings often point to + real bugs. `Many of them <https://searchfox.org/mozilla-central/source/build/moz.configure/warnings.configure>`__ + are enabled by default in the build system. +- In C++ code, use ``nullptr`` for pointers. In C code, using ``NULL`` + or ``0`` is allowed. + +.. note:: + + For the C++ rule, clang-tidy provides the ``modernize-use-nullptr`` check + with autofixes. + +- Don't use ``PRBool`` and ``PRPackedBool`` in C++, use ``bool`` + instead. +- For checking if a ``std`` container has no items, don't use + ``size()``, instead use ``empty()``. +- When testing a pointer, use ``(!myPtr)`` or ``(myPtr)``; + don't use ``myPtr != nullptr`` or ``myPtr == nullptr``. +- Do not compare ``x == true`` or ``x == false``. Use ``(x)`` or + ``(!x)`` instead. ``if (x == true)`` may have semantics different from + ``if (x)``! + +.. note:: + + clang-tidy provides the ``readability-simplify-boolean-expr`` check + with autofixes that checks for these and some other boolean expressions + that can be simplified. + +- In general, initialize variables with ``nsFoo aFoo = bFoo,`` and not + ``nsFoo aFoo(bFoo)``. + + - For constructors, initialize member variables with : ``nsFoo + aFoo(bFoo)`` syntax. + +- To avoid warnings created by variables used only in debug builds, use + the + `DebugOnly<T> <https://developer.mozilla.org/docs/Mozilla/Debugging/DebugOnly%3CT%3E>`__ + helper when declaring them. +- You should `use the static preference + API <https://firefox-source-docs.mozilla.org/modules/libpref/index.html>`__ for + working with preferences. +- One-argument constructors, that are not copy or move constructors, + should generally be marked explicit. Exceptions should be annotated + with ``MOZ_IMPLICIT``. +- Use ``char32_t`` as the return type or argument type of a method that + returns or takes as argument a single Unicode scalar value. (Don't + use UTF-32 strings, though.) +- Prefer unsigned types for semantically-non-negative integer values. +- When operating on integers that could overflow, use ``CheckedInt``. +- Avoid the usage of ``typedef``, instead, please use ``using`` instead. + +.. note:: + + For parts of this rule, clang-tidy provides the ``modernize-use-using`` + check with autofixes. + + +Header files +------------ + +Since the Firefox code base is huge and uses a monolithic build, it is +of utmost importance for keeping build times reasonable to limit the +number of included files in each translation unit to the required minimum. +Exported header files need particular attention in this regard, since their +included files propagate, and many of them are directly or indirectly +included in a large number of translation units. + +- Include guards are named per the Google coding style (i.e. upper snake + case with a single trailing underscore). They should not include a + leading ``MOZ_`` or ``MOZILLA_``. For example, ``dom/media/foo.h`` + would use the guard ``DOM_MEDIA_FOO_H_``. +- Forward-declare classes in your header files, instead of including + them, whenever possible. For example, if you have an interface with a + ``void DoSomething(nsIContent* aContent)`` function, forward-declare + with ``class nsIContent;`` instead of ``#include "nsIContent.h"``. + If a "forwarding header" is provided for a type, include that instead of + putting the literal forward declaration(s) in your header file. E.g. for + some JavaScript types, there is ``js/TypeDecls.h``, for the string types + there is ``StringFwd.h``. One reason for this is that this allows + changing a type to a type alias by only changing the forwarding header. + The following uses of a type can be done with a forward declaration only: + + - Parameter or return type in a function declaration + - Member/local variable pointer or reference type + - Use as a template argument (not in all cases) in a member/local variable type + - Defining a type alias + + The following uses of a type require a full definition: + + - Base class + - Member/local variable type + - Use with delete or new + - Use as a template argument (not in all cases) + - Any uses of non-scoped enum types + - Enum values of a scoped enum type + + Use as a template argument is somewhat tricky. It depends on how the + template uses the type. E.g. ``mozilla::Maybe<T>`` and ``AutoTArray<T>`` + always require a full definition of ``T`` because the size of the + template instance depends on the size of ``T``. ``RefPtr<T>`` and + ``UniquePtr<T>`` don't require a full definition (because their + pointer member always has the same size), but their destructor + requires a full definition. If you encounter a template that cannot + be instantiated with a forward declaration only, but it seems + it should be possible, please file a bug (if it doesn't exist yet). + + Therefore, also consider the following guidelines to allow using forward + declarations as widely as possible. +- Inline function bodies in header files often pull in a lot of additional + dependencies. Be mindful when adding or extending inline function bodies, + and consider moving the function body to the cpp file or to a separate + header file that is not included everywhere. Bug 1677553 intends to provide + a more specific guideline on this. +- Consider the use of the `Pimpl idiom <https://en.cppreference.com/w/cpp/language/pimpl>`__, + i.e. hide the actual implementation in a separate ``Impl`` class that is + defined in the implementation file and only expose a ``class Impl;`` forward + declaration and ``UniquePtr<Impl>`` member in the header file. +- Do not use non-scoped enum types. These cannot be forward-declared. Use + scoped enum types instead, and forward declare them when possible. +- Avoid nested types that need to be referenced from outside the class. + These cannot be forward declared. Place them in a namespace instead, maybe + in an extra inner namespace, and forward declare them where possible. +- Avoid mixing declarations with different sets of dependencies in a single + header file. This is generally advisable, but even more so when some of these + declarations are used by a subset of the translation units that include the + combined header file only. Consider such a badly mixed header file like: + + .. code-block:: cpp + + /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=8 sts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + + #ifndef BAD_MIXED_FILE_H_ + #define BAD_MIXED_FILE_H_ + + // Only this include is needed for the function declaration below. + #include "nsCOMPtr.h" + + // These includes are only needed for the class definition. + #include "nsIFile.h" + #include "mozilla/ComplexBaseClass.h" + + namespace mozilla { + + class WrappedFile : public nsIFile, ComplexBaseClass { + // ... class definition left out for clarity + }; + + // Assuming that most translation units that include this file only call + // the function, but don't need the class definition, this should be in a + // header file on its own in order to avoid pulling in the other + // dependencies everywhere. + nsCOMPtr<nsIFile> CreateDefaultWrappedFile(nsCOMPtr<nsIFile>&& aFileToWrap); + + } // namespace mozilla + + #endif // BAD_MIXED_FILE_H_ + + +An example header file based on these rules (with some extra comments): + +.. code-block:: cpp + + /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=8 sts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + + #ifndef DOM_BASE_FOO_H_ + #define DOM_BASE_FOO_H_ + + // Include guards should come at the very beginning and always use exactly + // the style above. Otherwise, compiler optimizations that avoid rescanning + // repeatedly included headers might not hit and cause excessive compile + // times. + + #include <cstdint> + #include "nsCOMPtr.h" // This is needed because we have a nsCOMPtr<T> data member. + + class nsIFile; // Used as a template argument only. + enum class nsresult : uint32_t; // Used as a parameter type only. + template <class T> + class RefPtr; // Used as a return type only. + + namespace mozilla::dom { + + class Document; // Used as a template argument only. + + // Scoped enum, not as a nested type, so it can be + // forward-declared elsewhere. + enum class FooKind { Small, Big }; + + class Foo { + public: + // Do not put the implementation in the header file, it would + // require including nsIFile.h + Foo(nsCOMPtr<nsIFile> aFile, FooKind aFooKind); + + RefPtr<Document> CreateDocument(); + + void SetResult(nsresult aResult); + + // Even though we will default this destructor, do this in the + // implementation file since we would otherwise need to include + // nsIFile.h in the header. + ~Foo(); + + private: + nsCOMPtr<nsIFile> mFile; + }; + + } // namespace mozilla::dom + + #endif // DOM_BASE_FOO_H_ + + +Corresponding implementation file: + +.. code-block:: cpp + + /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */ + /* vim: set ts=8 sts=2 et sw=2 tw=80: */ + /* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + + #include "mozilla/dom/Foo.h" // corresponding header + + #include "mozilla/Assertions.h" // Needed for MOZ_ASSERT. + #include "mozilla/dom/Document.h" // Needed because we construct a Document. + #include "nsError.h" // Needed because we use NS_OK aka nsresult::NS_OK. + #include "nsIFile.h" // This is needed because our destructor indirectly calls delete nsIFile in a template instance. + + namespace mozilla::dom { + + // Do not put the implementation in the header file, it would + // require including nsIFile.h + Foo::Foo(nsCOMPtr<nsIFile> aFile, FooKind aFooKind) + : mFile{std::move(aFile)} { + } + + RefPtr<Document> Foo::CreateDocument() { + return MakeRefPtr<Document>(); + } + + void Foo::SetResult(nsresult aResult) { + MOZ_ASSERT(aResult != NS_OK); + + // do something with aResult + } + + // Even though we default this destructor, do this in the + // implementation file since we would otherwise need to include + // nsIFile.h in the header. + Foo::~Foo() = default; + + } // namespace mozilla::dom + + +Include directives +------------------ + +- Ordering: + + - In an implementation file (cpp file), the very first include directive + should include the corresponding header file, followed by a blank line. + - Any conditional includes (depending on some ``#ifdef`` or similar) follow + after non-conditional includes. Don't mix them in. + - Don't place comments between non-conditional includes. + + Bug 1679522 addresses automating the ordering via clang-format, which + is going to enforce some stricter rules. Expect the includes to be reordered. + If you include third-party headers that are not self-contained, and therefore + need to be included in a particular order, enclose those (and only those) + between ``// clang-format off`` and ``// clang-format on``. This should not be + done for Mozilla headers, which should rather be made self-contained if they + are not. + +- Brackets vs. quotes: C/C++ standard library headers are included using + brackets (e.g. ``#include <cstdint>``), all other include directives use + (double) quotes (e.g. ``#include "mozilla/dom/Document.h``). +- Exported headers should always be included from their exported path, not + from their source path in the tree, even if available locally. E.g. always + do ``#include "mozilla/Vector.h"``, not ``#include "Vector.h"``, even + from within `mfbt`. +- Generally, you should include exactly those headers that are needed, not + more and not less. Unfortunately this is not easy to see. Maybe C++20 + modules will bring improvements to this, but it will take a long time + to be adopted. +- The basic rule is that if you literally use a symbol in your file that + is declared in a header A.h, include that header. In particular in header + files, check if a forward declaration or including a forwarding header is + sufficient, see section :ref:`Header files`. + + There are cases where this basic rule is not sufficient. Some cases where + you need to include additional headers are: + + - You reference a member of a type that is not literally mentioned in your + code, but, e.g. is the return type of a function you are calling. + + There are also cases where the basic rule leads to redundant includes. Note + that "redundant" here does not refer to "accidentally redundant" headers, + e.g. at the time of writing ``mozilla/dom/BodyUtil.h`` includes + ``mozilla/dom/FormData.h``, but it doesn't need to (it only needs a forward + declaration), so including ``mozilla/dom/FormData.h`` is "accidentally + redundant" when including ``mozilla/dom/BodyUtil.h``. The includes of + ``mozilla/dom/BodyUtil.h`` might change at any time, so if a file that + includes ``mozilla/dom/BodyUtil.h`` needs a full definition of + ``mozilla::dom::FormData``, it should includes ``mozilla/dom/FormData.h`` + itself. In fact, these "accidentally redundant" headers MUST be included. + Relying on accidentally redundant includes makes any change to a header + file extremely hard, in particular when considering that the set of + accidentally redundant includes differs between platforms. + But some cases in fact are non-accidentally redundant, and these can and + typically should not be repeated: + + - The includes of the header file do not need to be repeated in its + corresponding implementation file. Rationale: the implementation file and + its corresponding header file are tightly coupled per se. + + Macros are a special case. Generally, the literal rule also applies here, + i.e. if the macro definition references a symbol, the file containing the + macro definition should include the header defining the symbol. E.g. + ``NS_IMPL_CYCLE_COLLECTING_NATIVE_RELEASE`` defined in ``nsISupportsImpl.h`` + makes use of ``MOZ_ASSERT`` defined in ``mozilla/Assertions.h``, so + ``nsISupportsImpl.h`` includes ``mozilla/Assertions.h``. However, this + requires human judgment of what is intended, since technically only the + invocations of the macro reference a symbol (and that's how + include-what-you-use handles this). It might depend on the + context or parameters which symbol is actually referenced, and sometimes + this is on purpose. In these cases, the user of the macro needs to include + the required header(s). + + + +COM and pointers +---------------- + +- Use ``nsCOMPtr<>`` + If you don't know how to use it, start looking in the code for + examples. The general rule, is that the very act of typing + ``NS_RELEASE`` should be a signal to you to question your code: + "Should I be using ``nsCOMPtr`` here?". Generally the only valid use + of ``NS_RELEASE`` is when you are storing refcounted pointers in a + long-lived datastructure. +- Declare new XPCOM interfaces using :doc:`XPIDL </xpcom/xpidl>`, so they + will be scriptable. +- Use :doc:`nsCOMPtr </xpcom/refptr>` for strong references, and + ``nsWeakPtr`` for weak references. +- Don't use ``QueryInterface`` directly. Use ``CallQueryInterface`` or + ``do_QueryInterface`` instead. +- Use :ref:`Contract IDs <contract_ids>`, + instead of CIDs with ``do_CreateInstance``/``do_GetService``. +- Use pointers, instead of references for function out parameters, even + for primitive types. + + +IDL +--- + + +Use leading-lowercase, or "interCaps" +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When defining a method or attribute in IDL, the first letter should be +lowercase, and each following word should be capitalized. For example: + +.. code-block:: cpp + + long updateStatusBar(); + + +Use attributes wherever possible +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Whenever you are retrieving or setting a single value, without any +context, you should use attributes. Don't use two methods when you could +use an attribute. Using attributes logically connects the getting and +setting of a value, and makes scripted code look cleaner. + +This example has too many methods: + +.. code-block:: cpp + + interface nsIFoo : nsISupports + { + long getLength(); + void setLength(in long length); + long getColor(); + }; + +The code below will generate the exact same C++ signature, but is more +script-friendly. + +.. code-block:: cpp + + interface nsIFoo : nsISupports + { + attribute long length; + readonly attribute long color; + }; + + +Use Java-style constants +~~~~~~~~~~~~~~~~~~~~~~~~ + +When defining scriptable constants in IDL, the name should be all +uppercase, with underscores between words: + +.. code-block:: cpp + + const long ERROR_UNDEFINED_VARIABLE = 1; + + +See also +~~~~~~~~ + +For details on interface development, as well as more detailed style +guides, see the `Interface development +guide <https://developer.mozilla.org/docs/Mozilla/Developer_guide/Interface_development_guide>`__. + + +Error handling +-------------- + + +Check for errors early and often +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Every time you make a call into an XPCOM function, you should check for +an error condition. You need to do this even if you know that call will +never fail. Why? + +- Someone may change the callee in the future to return a failure + condition. +- The object in question may live on another thread, another process, + or possibly even another machine. The proxy could have failed to make + your call in the first place. + +Also, when you make a new function which is failable (i.e. it will +return a ``nsresult`` or a ``bool`` that may indicate an error), you should +explicitly mark the return value should always be checked. For example: + +:: + + // for IDL. + [must_use] nsISupports + create(); + + // for C++, add this in *declaration*, do not add it again in implementation. + [[nodiscard]] nsresult + DoSomething(); + +There are some exceptions: + +- Predicates or getters, which return ``bool`` or ``nsresult``. +- IPC method implementation (For example, ``bool RecvSomeMessage()``). +- Most callers will check the output parameter, see below. + +.. code-block:: cpp + + nsresult + SomeMap::GetValue(const nsString& key, nsString& value); + +If most callers need to check the output value first, then adding +``[[nodiscard]]`` might be too verbose. In this case, change the return value +to void might be a reasonable choice. + +There is also a static analysis attribute ``[[nodiscard]]``, which can +be added to class declarations, to ensure that those declarations are +always used when they are returned. + + +Use the NS_WARN_IF macro when errors are unexpected. +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +The ``NS_WARN_IF`` macro can be used to issue a console warning, in debug +builds if the condition fails. This should only be used when the failure +is unexpected and cannot be caused by normal web content. + +If you are writing code which wants to issue warnings when methods fail, +please either use ``NS_WARNING`` directly, or use the new ``NS_WARN_IF`` macro. + +.. code-block:: cpp + + if (NS_WARN_IF(somethingthatshouldbefalse)) { + return NS_ERROR_INVALID_ARG; + } + + if (NS_WARN_IF(NS_FAILED(rv))) { + return rv; + } + +Previously, the ``NS_ENSURE_*`` macros were used for this purpose, but +those macros hide return statements, and should not be used in new code. +(This coding style rule isn't generally agreed, so use of ``NS_ENSURE_*`` +can be valid.) + + +Return from errors immediately +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In most cases, your knee-jerk reaction should be to return from the +current function, when an error condition occurs. Don't do this: + +.. code-block:: cpp + + rv = foo->Call1(); + if (NS_SUCCEEDED(rv)) { + rv = foo->Call2(); + if (NS_SUCCEEDED(rv)) { + rv = foo->Call3(); + } + } + return rv; + +Instead, do this: + +.. code-block:: cpp + + rv = foo->Call1(); + if (NS_FAILED(rv)) { + return rv; + } + + rv = foo->Call2(); + if (NS_FAILED(rv)) { + return rv; + } + + rv = foo->Call3(); + if (NS_FAILED(rv)) { + return rv; + } + +Why? Error handling should not obfuscate the logic of the code. The +author's intent, in the first example, was to make 3 calls in +succession. Wrapping the calls in nested if() statements, instead +obscured the most likely behavior of the code. + +Consider a more complicated example to hide a bug: + +.. code-block:: cpp + + bool val; + rv = foo->GetBooleanValue(&val); + if (NS_SUCCEEDED(rv) && val) { + foo->Call1(); + } else { + foo->Call2(); + } + +The intent of the author, may have been, that ``foo->Call2()`` would only +happen when val had a false value. In fact, ``foo->Call2()`` will also be +called, when ``foo->GetBooleanValue(&val)`` fails. This may, or may not, +have been the author's intent. It is not clear from this code. Here is +an updated version: + +.. code-block:: cpp + + bool val; + rv = foo->GetBooleanValue(&val); + if (NS_FAILED(rv)) { + return rv; + } + if (val) { + foo->Call1(); + } else { + foo->Call2(); + } + +In this example, the author's intent is clear, and an error condition +avoids both calls to ``foo->Call1()`` and ``foo->Call2();`` + +*Possible exceptions:* Sometimes it is not fatal if a call fails. For +instance, if you are notifying a series of observers that an event has +fired, it might be trivial that one of these notifications failed: + +.. code-block:: cpp + + for (size_t i = 0; i < length; ++i) { + // we don't care if any individual observer fails + observers[i]->Observe(foo, bar, baz); + } + +Another possibility, is you are not sure if a component exists or is +installed, and you wish to continue normally, if the component is not +found. + +.. code-block:: cpp + + nsCOMPtr<nsIMyService> service = do_CreateInstance(NS_MYSERVICE_CID, &rv); + // if the service is installed, then we'll use it. + if (NS_SUCCEEDED(rv)) { + // non-fatal if this fails too, ignore this error. + service->DoSomething(); + + // this is important, handle this error! + rv = service->DoSomethingImportant(); + if (NS_FAILED(rv)) { + return rv; + } + } + + // continue normally whether or not the service exists. + + +Strings +------- + +.. note:: + + This section overlaps with the more verbose advice given in + :doc:`String guide </xpcom/stringguide>`. + These should eventually be merged. For now, please refer to that guide for + more advice. + +- String arguments to functions should be declared as ``[const] nsA[C]String&``. +- Prefer using string literals. In particular, use empty string literals, + i.e. ``u""_ns`` or ``""_ns``, instead of ``Empty[C]String()`` or + ``const nsAuto[C]String empty;``. Use ``Empty[C]String()`` only if you + specifically need a ``const ns[C]String&``, e.g. with the ternary operator + or when you need to return/bind to a reference or take the address of the + empty string. +- For 16-bit literal strings, use ``u"..."_ns`` or, if necessary + ``NS_LITERAL_STRING_FROM_CSTRING(...)`` instead of ``nsAutoString()`` + or other ways that would do a run-time conversion. + See :ref:`Avoid runtime conversion of string literals <Avoid runtime conversion of string literals>` below. +- To compare a string with a literal, use ``.EqualsLiteral("...")``. +- Use ``str.IsEmpty()`` instead of ``str.Length() == 0``. +- Use ``str.Truncate()`` instead of ``str.SetLength(0)``, + ``str.Assign(""_ns)`` or ``str.AssignLiteral("")``. +- Don't use functions from ``ctype.h`` (``isdigit()``, ``isalpha()``, + etc.) or from ``strings.h`` (``strcasecmp()``, ``strncasecmp()``). + These are locale-sensitive, which makes them inappropriate for + processing protocol text. At the same time, they are too limited to + work properly for processing natural-language text. Use the + alternatives in ``mozilla/TextUtils.h`` and in ``nsUnicharUtils.h`` + in place of ``ctype.h``. In place of ``strings.h``, prefer the + ``nsStringComparator`` facilities for comparing strings or if you + have to work with zero-terminated strings, use ``nsCRT.h`` for + ASCII-case-insensitive comparison. + + +Use the ``Auto`` form of strings for local values +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +When declaring a local, short-lived ``nsString`` class, always use +``nsAutoString`` or ``nsAutoCString``. These pre-allocate a 64-byte +buffer on the stack, and avoid fragmenting the heap. Don't do this: + +.. code-block:: cpp + + nsresult + foo() + { + nsCString bar; + .. + } + +instead: + +.. code-block:: cpp + + nsresult + foo() + { + nsAutoCString bar; + .. + } + + +Be wary of leaking values from non-XPCOM functions that return char\* or PRUnichar\* +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +It is an easy trap to return an allocated string, from an internal +helper function, and then using that function inline in your code, +without freeing the value. Consider this code: + +.. code-block:: cpp + + static char* + GetStringValue() + { + .. + return resultString.ToNewCString(); + } + + .. + WarnUser(GetStringValue()); + +In the above example, ``WarnUser`` will get the string allocated from +``resultString.ToNewCString()`` and throw away the pointer. The +resulting value is never freed. Instead, either use the string classes, +to make sure your string is automatically freed when it goes out of +scope, or make sure that your string is freed. + +Automatic cleanup: + +.. code-block:: cpp + + static void + GetStringValue(nsAWritableCString& aResult) + { + .. + aResult.Assign("resulting string"); + } + + .. + nsAutoCString warning; + GetStringValue(warning); + WarnUser(warning.get()); + +Free the string manually: + +.. code-block:: cpp + + static char* + GetStringValue() + { + .. + return resultString.ToNewCString(); + } + + .. + char* warning = GetStringValue(); + WarnUser(warning); + nsMemory::Free(warning); + +.. _Avoid runtime conversion of string literals: + +Avoid runtime conversion of string literals +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +It is very common to need to assign the value of a literal string, such +as ``"Some String"``, into a unicode buffer. Instead of using ``nsString``'s +``AssignLiteral`` and ``AppendLiteral``, use a user-defined literal like `u"foo"_ns` +instead. On most platforms, this will force the compiler to compile in a +raw unicode string, and assign it directly. In cases where the literal is defined +via a macro that is used in both 8-bit and 16-bit ways, you can use +`NS_LITERAL_STRING_FROM_CSTRING` to do the conversion at compile time. + +Incorrect: + +.. code-block:: cpp + + nsAutoString warning; + warning.AssignLiteral("danger will robinson!"); + ... + foo->SetStringValue(warning); + ... + bar->SetUnicodeValue(warning.get()); + +Correct: + +.. code-block:: cpp + + constexpr auto warning = u"danger will robinson!"_ns; + ... + // if you'll be using the 'warning' string, you can still use it as before: + foo->SetStringValue(warning); + ... + bar->SetUnicodeValue(warning.get()); + + // alternatively, use the wide string directly: + foo->SetStringValue(u"danger will robinson!"_ns); + ... + + // if a macro is the source of a 8-bit literal and you cannot change it, use + // NS_LITERAL_STRING_FROM_CSTRING, but only if necessary. + #define MY_MACRO_LITERAL "danger will robinson!" + foo->SetStringValue(NS_LITERAL_STRING_FROM_CSTRING(MY_MACRO_LITERAL)); + + // If you need to pass to a raw const char16_t *, there's no benefit to + // go through our string classes at all, just do... + bar->SetUnicodeValue(u"danger will robinson!"); + + // .. or, again, if a macro is the source of a 8-bit literal + bar->SetUnicodeValue(u"" MY_MACRO_LITERAL); + + +Usage of PR_(MAX|MIN|ABS|ROUNDUP) macro calls +--------------------------------------------- + +Use the standard-library functions (``std::max``), instead of +``PR_(MAX|MIN|ABS|ROUNDUP)``. + +Use ``mozilla::Abs`` instead of ``PR_ABS``. All ``PR_ABS`` calls in C++ code have +been replaced with ``mozilla::Abs`` calls, in `bug +847480 <https://bugzilla.mozilla.org/show_bug.cgi?id=847480>`__. All new +code in ``Firefox/core/toolkit`` needs to ``#include "nsAlgorithm.h"`` and +use the ``NS_foo`` variants instead of ``PR_foo``, or +``#include "mozilla/MathAlgorithms.h"`` for ``mozilla::Abs``. + +Use of SpiderMonkey rooting typedefs +------------------------------------ +The rooting typedefs in ``js/public/TypeDecls.h``, such as ``HandleObject`` and +``RootedObject``, are deprecated both in and outside of SpiderMonkey. They will +eventually be removed and should not be used in new code. |