Chromium Code Reviews| OLD | NEW |
|---|---|
| (Empty) | |
| 1 # Chromium C++ style guide | |
| 2 | |
| 3 _For other languages, please see the [Chromium style guides](https://chromium.go oglesource.com/chromium/src/+/master/styleguide/styleguide.md)._ | |
| 4 | |
| 5 Chromium follows the [Google C++ Style | |
| 6 Guide](https://google.github.io/styleguide/cppguide.html) unless an exception | |
| 7 is listed below. | |
| 8 | |
| 9 A checkout should give you | |
| 10 [clang-format](https://chromium.googlesource.com/chromium/src/+/master/docs/clan g_format.md) | |
| 11 to automatically format C++ code. By policy, Clang's formatting of code should | |
| 12 always be accepted in code reviews. | |
| 13 | |
| 14 You can propose changes to the style guide by sending an email to | |
| 15 `cxx@chromium.org`. Ideally, the list will arrive at some consensus and the | |
| 16 wiki page will be updated to mention that consensus. If there's no consensus, | |
| 17 `src/styleguide/c++/OWNERS` get to decide. | |
| 18 | |
| 19 Blink code in `third_party/WebKit` uses [Blink | |
| 20 style](https://sites.google.com/a/chromium.org/dev/blink/coding-style). | |
| 21 | |
| 22 ## C++11 features | |
| 23 | |
| 24 Google style has adopted most C++11 features, but Chromium has a more | |
| 25 restricted set. The status of C++11 features in Chromium is tracked in the | |
| 26 separate [C++11 use in Chromium](https://chromium-cpp.appspot.com/) page. | |
| 27 | |
| 28 ## Naming | |
| 29 | |
| 30 * "Chromium" is the name of the project, not the product, and should never | |
| 31 appear in code, variable names, API names etc. Use "Chrome" instead. | |
| 32 | |
| 33 * Though the Google C++ Style Guide now says to use `kConstantNaming` for | |
| 34 enums, Chromium was written using `MACRO_STYLE` naming. In enums that are | |
| 35 actually enumerations (i.e. have multiple values), continue to use this | |
| 36 style for consistency. Use `kConstantNaming` when using the "enum hack" to | |
| 37 define a single constant, as you would for a const int or the like. | |
| 38 | |
| 39 * Functions used only for testing should be restricted to test-only scenarios | |
| 40 either by `#ifdefing` them appropriately (e.g. `#if defined(UNIT_TEST)`) or | |
| 41 by naming them with a `ForTesting` suffix. The latter will be checked at | |
| 42 presubmit time to ensure they're only called by test files. | |
| 43 | |
| 44 ## Code formatting | |
| 45 | |
| 46 * Put `*` and `&` by the type rather than the variable name. | |
| 47 | |
| 48 * When you derive from a base class, group any overriding functions in your | |
| 49 header file in one labeled section. Use the override specifier on all these | |
| 50 functions. | |
| 51 | |
| 52 * Prefer `(foo == 0)` to `(0 == foo)`. | |
| 53 | |
| 54 * Function declaration order should match function definition order. | |
| 55 | |
| 56 * Prefer putting delegate classes in their own header files. Implementors of | |
| 57 the delegate interface will often be included elsewhere, which will often | |
| 58 cause more coupling with the header of the main class. | |
| 59 | |
| 60 * Don't use else after return. So use: | |
| 61 ```c++ | |
| 62 if (foo) | |
| 63 return 1; | |
| 64 return 2; | |
| 65 ``` | |
| 66 instead of: | |
| 67 ```c++ | |
| 68 if (foo) | |
| 69 return 1; | |
| 70 else | |
| 71 return 2; | |
| 72 ``` | |
| 73 | |
| 74 ## Unnamed namespaces | |
| 75 | |
| 76 Items local to a .cc file should be wrapped in an unnamed namespace. While some | |
| 77 such items are already file-scope by default in C++, not all are; also, shared | |
| 78 objects on Linux builds export all symbols, so unnamed namespaces (which | |
| 79 restrict these symbols to the compilation unit) improve function call cost and | |
| 80 reduce the size of entry point tables. | |
| 81 | |
| 82 ## Exporting symbols | |
| 83 | |
| 84 When building shared libraries and DLLs, we need to indicate which functions | |
| 85 and classes should be visible outside of the library, and which should only be | |
| 86 visible inside the library. | |
| 87 | |
| 88 Symbols can be exported by annotating with a `<COMPONENT>_EXPORT` macro name | |
| 89 (where `<COMPONENT>` is the name of the component being built, e.g. BASE, NET, | |
| 90 CONTENT, etc.). Class annotations should precede the class name: | |
| 91 ```c++ | |
| 92 class FOO_EXPORT Foo { | |
| 93 void Bar(); | |
| 94 void Baz(); | |
| 95 // ... | |
| 96 }; | |
| 97 ``` | |
| 98 | |
| 99 Function annotations should precede the return type: | |
| 100 ```c++ | |
| 101 class FooSingleton { | |
| 102 FOO_EXPORT Foo& GetFoo(); | |
| 103 FOO_EXPORT Foo& SetFooForTesting(Foo& foo); | |
| 104 void SetFoo(Foo& foo); | |
| 105 }; | |
| 106 ``` | |
| 107 | |
| 108 These examples result in `Foo::Bar()`, `Foo::Baz()`, `FooSingleton::GetFoo()`, | |
| 109 and `FooSingleton::SetFooForTesting()` all being available outside of the DLL, | |
| 110 but not `FooSingleton::SetFoo()`. | |
| 111 | |
| 112 Whether something is exported is distinct from whether it is public or private, | |
| 113 or even whether it would normally be considered part of the external API. For | |
| 114 example, if part of the external API is an inlined function that calls a | |
| 115 private function, that private function must be exported as well. | |
| 116 | |
| 117 ## Multiple inheritance | |
| 118 | |
| 119 Multiple inheritance and virtual inheritance are permitted in Chromium code, | |
| 120 but discouraged (beyond the "interface" style of inheritance allowed by the | |
| 121 Google style guide, for which we do not require classes to have the "Interface" | |
| 122 suffix). Consider whether composition could solve the problem instead. | |
| 123 | |
| 124 ## Inline functions | |
| 125 | |
| 126 Simple accessors should generally be the only inline functions. These should be | |
| 127 named `unix_hacker_style()`. Virtual functions should never be declared this way . | |
| 128 For more detail, consult the [C++ Dos and | |
| 129 Don'ts](https://www.chromium.org/developers/coding-style/cpp-dos-and-donts) | |
| 130 section on inlining. | |
| 131 | |
| 132 ## Logging | |
|
brettw
2016/07/12 20:19:45
Nico: This is what you're referring to I think. Th
| |
| 133 | |
| 134 Remove most logging calls before checking in. Unless you're adding temporary | |
| 135 logging to track down a specific bug, and you have a plan for how to collect | |
| 136 the logged data from user machines, you should generally not add logging | |
| 137 statements. | |
| 138 | |
| 139 For the rare case when logging needs to stay in the codebase for a while, | |
| 140 prefer `DVLOG(1)` to other logging methods. This avoids bloating the release | |
| 141 executable and in debug can be selectively enabled at runtime by command-line | |
| 142 arguments: | |
| 143 | |
| 144 * `--v=n` sets the global log level to n (default 0). All log statements with a | |
| 145 log level less than or equal to the global level will be printed. | |
| 146 | |
| 147 * `--vmodule=mod=n[,mod=n,...]` overrides the global log level for the module | |
| 148 mod. Supplying the string foo for mod will affect all files named foo.cc, | |
| 149 while supplying a wildcard like `*bar/baz*` will affect all files with | |
| 150 `bar/baz` in their full pathnames. | |
| 151 | |
| 152 ## Platform-specific code | |
| 153 | |
| 154 To `#ifdef` code for specific platforms, use the macros defined in | |
| 155 `build/build_config.h` and in the Chromium build config files, not other macros | |
| 156 set by specific compilers or build environments (e.g. `WIN32`). | |
| 157 | |
| 158 Place platform-specific #includes in their own section below the "normal" | |
| 159 `#includes`. Repeat the standard `#include` order within this section: | |
| 160 | |
| 161 ```c++ | |
| 162 #include "foo/foo.h" | |
| 163 | |
| 164 #include <stdint.h> | |
| 165 #include <algorithm> | |
| 166 | |
| 167 #include "base/strings/utf_string_conversions.h" | |
| 168 #include "chrome/common/render_messages.h" | |
| 169 | |
| 170 #if defined(OS_WIN) | |
| 171 #include <windows.h> | |
| 172 #include "base/win/scoped_comptr.h" | |
| 173 #elif defined(OS_POSIX) | |
| 174 #include "base/posix/global_descriptors.h" | |
| 175 #endif | |
| 176 ``` | |
| 177 | |
| 178 ## Types | |
| 179 | |
| 180 * Use `size_t` for object and allocation sizes, object counts, array and | |
| 181 pointer offsets, vector indices, and so on. The signed types are incorrect | |
| 182 and unsafe for these purposes (e.g. integer overflow behavior for signed | |
| 183 types is undefined in the C and C++ standards, while the behavior is | |
| 184 defined for unsigned types.) The C++ STL is a guide here: they use `size_t` | |
| 185 and `foo::size_type` for very good reasons. | |
| 186 | |
| 187 * Use `size_t` directly in preference to `std::string::size_type` and similar. | |
| 188 | |
| 189 * Occasionally classes may have a good reason to use a type other than `size_t ` | |
| 190 for one of these concepts, e.g. as a storage space optimization. In these | |
| 191 cases, continue to use `size_t` in public-facing function declarations. | |
| 192 | |
| 193 * Be aware that `size_t` (object sizes and indices), `off_t` (file offsets), | |
| 194 `ptrdiff_t` (the difference between two pointer values), `intptr_t` (an | |
| 195 integer type large enough to hold the value of a pointer), `uint32_t`, | |
| 196 `uint64_t`, and so on are not necessarily the same. Use the right type for | |
| 197 your purpose. | |
| 198 | |
| 199 * When casting to and from different types, use `static_cast<>()` when you kno w | |
| 200 the conversion is safe. Use `checked_cast<>()` (from | |
| 201 `base/numerics/safe_conversions.h`) when you need to enforce via `CHECK()` t hat | |
| 202 the source value is in-range for the destination type. Use | |
| 203 `saturated_cast<>()` (from the same file) if you instead wish to clamp | |
| 204 out-of-range values. | |
| 205 | |
| 206 * Do not use unsigned types to mean "this value should never be < 0". For | |
| 207 that, use assertions or run-time checks (as appropriate). | |
| 208 | |
| 209 * In cases where the exact size of the type matters (e.g. a 32-bit pixel | |
| 210 value, a bitmask, or a counter that has to be a particular width), use one | |
| 211 of the sized types from `<stdint.h>`, e.g. `uint32_t`. | |
| 212 | |
| 213 * When passing values across network or process boundaries, use | |
| 214 explicitly-sized types for safety, since the sending and receiving ends may | |
| 215 not have been compiled with the same sizes for things like int and | |
| 216 `size_t`. However, to the greatest degree possible, avoid letting these | |
| 217 sized types bleed through the APIs of the layers in question. | |
| 218 | |
| 219 * Don't use `std::wstring`. Use `base::string16` or `base::FilePath` instead. | |
| 220 (Windows-specific code interfacing with system APIs using `wstring` and | |
| 221 `wchar_t` can still use `string16` and `char16`; it is safe to assume that | |
| 222 these are equivalent to the "wide" types.) | |
| 223 | |
| 224 ## Object ownership and calling conventions | |
| 225 | |
| 226 When functions need to take raw or smart pointers as parameters, use the | |
| 227 following conventions. Here we refer to the parameter type as `T` and name as | |
| 228 `t`. | |
| 229 | |
| 230 * If the function does not modify `t`'s ownership, declare the param as `T*`. The | |
| 231 caller is expected to ensure `t` stays alive as long as necessary, generally | |
| 232 through the duration of the call. Exception: In rare cases (e.g. using | |
| 233 lambdas with STL algorithms over containers of `uniuqe_ptr<>`s), you may be | |
| 234 forced to declare the param as `const std::unique_ptr<T>&`. Do this only whe n | |
| 235 required. | |
| 236 | |
| 237 * If the function takes ownership of a non-refcounted object, declare the | |
| 238 param as `std::unique_ptr<T>`. | |
| 239 | |
| 240 * If the function (at least sometimes) takes a ref on a refcounted object, | |
| 241 declare the param as `scoped_refptr<T>`. The caller can decide | |
| 242 whether it wishes to transfer ownership (by calling `std::move(t)` when | |
| 243 passing `t`) or retain its ref (by simply passing t directly). | |
| 244 | |
| 245 * In short, functions should never take ownership of parameters passed as raw | |
| 246 pointers, and there should rarely be a need to pass smart pointers by const | |
| 247 ref. | |
| 248 | |
| 249 Conventions for return values are similar: return raw pointers when the caller | |
| 250 does not take ownership, and return smart pointers by value otherwise, | |
| 251 potentially in conjunction with `std::move()`. | |
| 252 | |
| 253 A great deal of Chromium code predates the above rules. In particular, some | |
| 254 functions take ownership of params passed as `T*`, or take `const | |
| 255 scoped_refptr<T>&` instead of `T*`, or return `T*` instead of | |
| 256 `scoped_refptr<T>` (to avoid refcount churn pre-C++11). Try to clean up such | |
| 257 code when you find it, or at least not make such usage any more widespread. | |
| 258 | |
| 259 ## Forward declarations vs. #includes | |
| 260 | |
| 261 Unlike the Google style guide, Chromium style prefers forward declarations to | |
| 262 `#includes` where possible. This can reduce compile times and result in fewer | |
| 263 files needing recompilation when a header changes. | |
| 264 | |
| 265 You can and should use forward declarations for most types passed or returned | |
| 266 by value, reference, or pointer, or types stored as pointer members or in most | |
| 267 STL containers. However, if it would otherwise make sense to use a type as a | |
| 268 member by-value, don't convert it to a pointer just to be able to | |
| 269 forward-declare the type. | |
| 270 | |
| 271 ## File headers | |
| 272 | |
| 273 All files in Chromium start with a common license header. That header should loo k like this: | |
| 274 | |
| 275 ```c++ | |
| 276 // Copyright $YEAR The Chromium Authors. All rights reserved. | |
| 277 // Use of this source code is governed by a BSD-style license that can be | |
| 278 // found in the LICENSE file. | |
| 279 ``` | |
| 280 | |
| 281 Some important notes about this header: | |
| 282 | |
| 283 * There is no `(c)` after `Copyright`. | |
| 284 | |
| 285 * `$YEAR` should be set to the current year at the time a file is created, and not changed thereafter. | |
| 286 | |
| 287 * For files specific to Chromium OS, replace the word Chromium with the phrase Chromium OS. | |
| 288 | |
| 289 * If the style changes, don't bother to update existing files to comply with | |
| 290 the new style. For the same reason, don't just blindly copy an existing | |
| 291 file's header when creating a new file, since the existing file may use an | |
| 292 outdated style. | |
| 293 | |
| 294 * The Chromium project hosts mirrors of some upstream open-source projects. | |
| 295 When contributing to these portions of the repository, retain the existing | |
| 296 file headers. | |
| 297 | |
| 298 Use standard `#include` guards in all header files (see the Google style guide | |
| 299 sections on these for the naming convention). Do not use `#pragma once`; | |
| 300 historically it was not supported on all platforms, and it does not seem to | |
| 301 outperform #include guards even on platforms which do support it. | |
| 302 | |
| 303 ## CHECK(), DCHECK(), and NOTREACHED() | |
| 304 | |
| 305 The `CHECK()` macro will cause an immediate crash if its condition is not met. | |
| 306 `DCHECK()` is like `CHECK()` but is only compiled in when `DCHECK_IS_ON` is true | |
| 307 (debug builds and some bot configurations, but not end-user builds). | |
| 308 `NOTREACHED()` is equivalent to `DCHECK(false)`. Here are some rules for using | |
| 309 these: | |
| 310 | |
| 311 * Use `DCHECK()` or `NOTREACHED()` as assertions, e.g. to document pre- and | |
| 312 post-conditions. A `DCHECK()` means "this condition must always be true", | |
| 313 not "this condition is normally true, but perhaps not in exceptional | |
| 314 cases." Things like disk corruption or strange network errors are examples | |
| 315 of exceptional circumstances that nevertheless should not result in | |
| 316 `DCHECK()` failure. | |
| 317 | |
| 318 * A consequence of this is that you should not handle DCHECK() failures, even | |
| 319 if failure would result in a crash. Attempting to handle a `DCHECK()` failur e | |
| 320 is a statement that the `DCHECK()` can fail, which contradicts the point of | |
| 321 writing the `DCHECK()`. In particular, do not write code like the following: | |
| 322 ```c++ | |
| 323 DCHECK(foo); | |
| 324 if (!foo) ... // Can't succeed! | |
| 325 | |
| 326 if (!bar) { | |
| 327 NOTREACHED(); | |
| 328 return; // Replace this whole conditional with "DCHECK(bar);" and keep going instead. | |
| 329 } | |
| 330 ``` | |
| 331 | |
| 332 * Use `CHECK()` if the consequence of a failed assertion would be a security | |
| 333 vulnerability, where crashing the browser is preferable. Because this takes | |
| 334 down the whole browser, sometimes there are better options than `CHECK()`. | |
| 335 For example, if a renderer sends the browser process a malformed IPC, an | |
| 336 attacker may control the renderer, but we can simply kill the offending | |
| 337 renderer instead of crashing the whole browser. | |
| 338 | |
| 339 * You can temporarily use `CHECK()` instead of `DCHECK()` when trying to | |
| 340 force crashes in release builds to sniff out which of your assertions is | |
| 341 failing. Don't leave these in the codebase forever; remove them or change | |
| 342 them back once you've solved the problem. | |
| 343 | |
| 344 * Don't use these macros in tests, as they crash the test binary and leave | |
| 345 bots in a bad state. Use the `ASSERT_xx()` and `EXPECT_xx()` family of | |
| 346 macros, which report failures gracefully and can continue running other | |
| 347 tests. | |
| 348 | |
| 349 ## Miscellany | |
| 350 | |
| 351 * Use UTF-8 file encodings and LF line endings. | |
| 352 | |
| 353 * Unit tests and performance tests should be placed in the same directory as | |
| 354 the functionality they're testing. | |
| 355 | |
| 356 * The [C++ do's and | |
| 357 don'ts](https://sites.google.com/a/chromium.org/dev/developers/coding-style/ cpp-dos-and-donts) | |
| 358 page has more helpful information. | |
| OLD | NEW |