summaryrefslogtreecommitdiff
path: root/chromium/styleguide
diff options
context:
space:
mode:
authorAllan Sandfeld Jensen <allan.jensen@qt.io>2019-07-31 15:50:41 +0200
committerAllan Sandfeld Jensen <allan.jensen@qt.io>2019-08-30 12:35:23 +0000
commit7b2ffa587235a47d4094787d72f38102089f402a (patch)
tree30e82af9cbab08a7fa028bb18f4f2987a3f74dfa /chromium/styleguide
parentd94af01c90575348c4e81a418257f254b6f8d225 (diff)
downloadqtwebengine-chromium-7b2ffa587235a47d4094787d72f38102089f402a.tar.gz
BASELINE: Update Chromium to 76.0.3809.94
Change-Id: I321c3f5f929c105aec0f98c5091ef6108822e647 Reviewed-by: Michael Brüning <michael.bruning@qt.io>
Diffstat (limited to 'chromium/styleguide')
-rw-r--r--chromium/styleguide/c++/blink-c++.md35
-rw-r--r--chromium/styleguide/c++/c++-dos-and-donts.md439
-rw-r--r--chromium/styleguide/c++/c++.md156
-rw-r--r--chromium/styleguide/c++/c++11.html24
-rw-r--r--chromium/styleguide/c++/const.md155
5 files changed, 236 insertions, 573 deletions
diff --git a/chromium/styleguide/c++/blink-c++.md b/chromium/styleguide/c++/blink-c++.md
index 2dc7fd4a3f4..db74e67a577 100644
--- a/chromium/styleguide/c++/blink-c++.md
+++ b/chromium/styleguide/c++/blink-c++.md
@@ -8,43 +8,20 @@ differs from Google style.
[TOC]
-## Use references for all non-null pointer arguments
-Pointer arguments that can never be null should be passed as a reference, even
-if this results in a mutable reference argument.
+## May use mutable reference arguments
-> Note: Even though Google style prohibits mutable reference arguments, Blink
-style explicitly permits their use.
+Mutable reference arguments are permitted in Blink, in contrast to Google style.
-**Good:**
+> Note: This rule is under [discussion](https://groups.google.com/a/chromium.org/d/msg/blink-dev/O7R4YwyPIHc/mJyEyJs-EAAJ).
+
+**OK:**
```c++
-// Passed by mutable reference since |frame| is assumed to be non-null.
+// May be passed by mutable reference since |frame| is assumed to be non-null.
FrameLoader::FrameLoader(LocalFrame& frame)
: frame_(&frame),
progress_tracker_(ProgressTracker::Create(frame)) {
// ...
}
-
-// Optional arguments should still be passed by pointer.
-void LocalFrame::SetDOMWindow(LocalDOMWindow* dom_window) {
- if (dom_window)
- GetScriptController().ClearWindowProxy();
-
- if (this->DomWindow())
- this->DomWindow()->Reset();
- dom_window_ = dom_window;
-}
-```
-
-**Bad:**
-```c++
-// Since the constructor assumes that |frame| is never null, it should be
-// passed as a mutable reference.
-FrameLoader::FrameLoader(LocalFrame* frame)
- : frame_(frame),
- progress_tracker_(ProgressTracker::Create(frame)) {
- DCHECK(frame_);
- // ...
-}
```
## Prefer WTF types over STL and base types
diff --git a/chromium/styleguide/c++/c++-dos-and-donts.md b/chromium/styleguide/c++/c++-dos-and-donts.md
index 9f5f4bf2e7a..2e342677199 100644
--- a/chromium/styleguide/c++/c++-dos-and-donts.md
+++ b/chromium/styleguide/c++/c++-dos-and-donts.md
@@ -6,245 +6,17 @@ Unlike the style guide, the content of this page is advisory, not required. You
can always deviate from something on this page, if the relevant
author/reviewer/OWNERS agree that another course is better.
-
## Minimize Code in Headers
-### Don't include unneeded headers
-
-If a file isn't using the symbols from some header, remove the header. It turns
-out that this happens frequently in the Chromium codebase due to refactoring.
-
-### Move helper / inner classes into the implementation
-
-You can also forward declare classes inside a class:
-
-```cpp
-class Whatever {
- public:
- /* ... */
- private:
- struct DataStruct;
- std::vector<DataStruct> data_;
-};
-```
-
-Any headers that DataStruct needed no longer need to be included in the header
-file and only need to be included in the implementation. This will often let you
-pull includes out of the header. For reference, the syntax in the implementation
-file is:
-
-```cpp
-struct Whatever::DataStruct {
-};
-```
-
-Note that sometimes you can't do this because certain STL data structures
-require the full definition at declaration time (most notably, std::deque and
-the STL adapters that wrap it).
-
-## Stop inlining code in headers
-
-*** note
-**BACKGROUND**: Unless something is a cheap accessor or you truly need it to be
-inlined, don't ask for it to be inlined. Remember that definitions inside class
-declarations are implicitly requested to be inlined.
-***
-
-DON'T:
-
-```cpp
-class InlinedMethods {
- InlinedMethods() {
- // This constructor is equivalent to having the inline keyword in front
- // of it!
- }
- void Method() {
- // Same here!
- }
-};
-```
-
-### Stop inlining complex methods
-
-DON'T:
-
-```cpp
-class DontDoThis {
- public:
- int ComputeSomething() {
- int sum = 0;
- for (int i = 0; i < limit; ++i) {
- sum += OtherMethod(i, ... );
- }
- return sum;
- }
-};
-```
-
-A request to inline is merely a suggestion to the compiler, and anything more
-than a few operations on integral data types will probably not be inlined.
-However, every file that has to use an inline method will also emit a function
-version in the resulting .o, even if the method was inlined. (This is to support
-function pointer behavior properly.) Therefore, by requesting an inline in this
-case, you're likely just adding crud to the .o files which the linker will need
-to do work to resolve.
-
-If the method has significant implementation, there's also a good chance that by
-not inlining it, you could eliminate some includes.
-
-### Stop inlining virtual methods
-
-You can't inline virtual methods under most circumstances, even if the method
-would otherwise be inlined because it's very short. The compiler must do runtime
-dispatch on any virtual method where the compiler doesn't know the object's
-complete type, which rules out the majority of cases where you have an object.
-
-### Stop inlining constructors and destructors
-
-Constructors and destructors are often significantly more complex than you think
-they are, especially if your class has any non-POD data members. Many STL
-classes have inlined constructors/destructors which may be copied into your
-function body. Because the bodies of these appear to be empty, they often seem
-like trivial functions that can safely be inlined. Don't give in to this
-temptation. Define them in the implementation file unless you really _need_
-them to be inlined. Even if they do nothing now, someone could later add
-something seemingly-trivial to the class and make your hundreds of inlined
-destructors much more complex.
-
-Even worse, inlining constructors/destructors prevents you from using forward
-declared variables.
-
-DON'T:
-
-```cpp
-class Forward;
-class WontCompile {
- public:
- // THIS WON'T COMPILE, BUT IT WOULD HAVE IF WE PUT THESE IN THE
- // IMPLEMENTATION FILE!
- //
- // The compiler needs the definition of Forward to call the
- // vector/scoped_ptr ctors/dtors.
- Example() { }
- ~Example() { }
-
- private:
- std::vector<Forward> examples_;
- scoped_ptr<Forward> super_example_;
-};
-```
-
-For more information, read Item 30 in Effective C++.
-
-### When you CAN inline constructors and destructors
-
-C++ has the concept of a
-[trivial destructor](http://publib.boulder.ibm.com/infocenter/macxhelp/v6v81/index.jsp?topic=/com.ibm.vacpp6m.doc/language/ref/clrc15cplr380.htm).
-If your class has only POD types and does not explicitly declare a destructor,
-then the compiler will not bother to generate or run a destructor.
-
-```cpp
-struct Data {
- Data() : count_one(0), count_two(0) {}
- // No explicit destructor, thus no implicit destructor either.
-
- // The members must all be POD for this trick to work.
- int count_one;
- int count_two;
-};
-```
-
-In this example, since there is no inheritance and only a few POD members, the
-constructor will be only a few trivial integer operations, and thus OK to
-inline.
-
-For abstract base classes with no members, it's safe to define the (trivial)
-destructor inline:
-
-```cpp
-class Interface {
- public:
- virtual ~Interface() {}
-
- virtual void DoSomething(int parameter) = 0;
- virtual int GetAValue() = 0;
-};
-```
-But be careful; these two "interfaces" don't count.
-
-DON'T:
-
-```cpp
-class ClaimsToBeAnInterface : public base::RefCounted<ClaimsToBeAnInterface> {
- public:
- virtual ~ClaimsToBeAnInterface() { /* But derives from a template! */ }
-};
-
-class HasARealMember {
- public:
- virtual void InterfaceMethod() = 0;
- virtual ~HasARealMember() {}
-
- protected:
- vector<string> some_data_;
-};
-```
-
-If in doubt, don't rely on these sorts of exceptions. Err on the side of not
-inlining.
-
-### Be careful about your accessors
-
-Not all accessors are light weight. Compare:
-
-```cpp
-class Foo {
- public:
- int count() const { return count_; }
-
- private:
- int count_;
-};
-```
-
-Here the accessor is trivial and safe to inline. But the following code is
-probably not, even though it also looks simple.
-
-DON'T:
-
-```cpp
-struct MyData {
- vector<GURL> urls_;
- base::Time last_access_;
-};
-
-class Manager {
- public:
- MyData get_data() { return my_data_; }
-
- private:
- MyData my_data_;
-};
-```
-
-The underlying copy constructor calls for MyData are going to be complex. (Also,
-they're going to be synthesized, which is bad.)
-
-### What about code outside of headers?
-
-For classes declared in .cc files, there's no risk of bloating several .o files
-with the definitions of the same "inlined" function. While there are other,
-weaker arguments to continue to avoid inlining, the primary remaining
-consideration is simply what would make code most readable.
-
-This is especially true in testing code. Test framework classes don't tend to
-be instantiated separately and passed around as objects; they're effectively
-just bundles of translation-unit-scope functionality coupled with a mechanism
-to reset state between tests. In these cases, defining the test functions
-inline at their declaration sites has little negative effect and can reduce the
-amount of "boilerplate" in the test file.
-
-Different reviewers may have different opinions here; use good judgment.
+* Remove #includes you don't use. Unfortunately, Chromium lacks
+ include-what-you-use ("IWYU") support, so there's no tooling to do this
+ automatically. Look carefully when refactoring.
+* Where possible, forward-declare nested classes, then give the full declaration
+ (and definition) in the .cc file.
+* Defining a class method in the declaration is an implicit request to inline
+ it. Avoid this in header files except for cheap non-virtual getters and
+ setters. Note that constructors and destructors can be more expensive than
+ they appear and should also generally not be inlined.
## Static variables
@@ -258,10 +30,10 @@ and
```cpp
void foo() {
- static int ok_count = ComputeTheCount(); // OK; a problem pre-2017.
- static int good_count = 42; // C++03 3.6.2 says this is done before dynamic initialization, so probably thread-safe.
- static constexpr int better_count = 42; // Even better, as this will likely be inlined at compile time.
- static auto& object = *new Object; // For class types.
+ static int ok_count = ComputeTheCount(); // OK; a problem pre-2017.
+ static int good_count = 42; // Done before dynamic initialization.
+ static constexpr int better_count = 42; // Even better (likely inlined at compile time).
+ static auto& object = *new Object; // For class types.
}
```
@@ -269,7 +41,6 @@ void foo() {
There are myriad ways to initialize variables in C++11. Prefer the following
general rules:
-
1. Use assignment syntax when performing "simple" initialization with one or
more literal values which will simply be composed into the object:
@@ -286,7 +57,6 @@ general rules:
happening. Note that "{}" are allowed on the right side of the '=' here
(e.g. when you're merely passing a set of initial values to a "simple"
struct/ container constructor; see below items for contrast).
-
2. Use constructor syntax when construction performs significant logic, uses an
explicit constructor, or in some other way is not intuitively "simple" to the
reader:
@@ -295,7 +65,6 @@ general rules:
MyClass c(1.7, false, "test");
std::vector<double> v(500, 0.97); // Creates 50 copies of the provided initializer
```
-
3. Use C++11 "uniform init" syntax ("{}" without '=') only when neither of the
above work:
@@ -321,7 +90,6 @@ general rules:
...
}
```
-
4. Never mix uniform init syntax with auto, since what it deduces is unlikely
to be what was intended:
@@ -357,17 +125,12 @@ PaddingValues GetPaddingValues() {
}
```
-In exchange for a longer definition, callers can now refer to a named type and
-named fields, and returning an instance of the struct is still (usually) terse.
-
A good rule of thumb for when to prefer a struct is whenever you'd find
declaring a type alias for the pair or tuple beneficial, which is usually
whenever it's used more than just as a local one-off.
## Use `std::make_unique` and `base::MakeRefCounted` instead of bare `new`
-Most Chromium contributors already understand the benefits of
-[using smart pointers to indicate ownership](http://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers).
When possible, avoid bare `new` by using
[`std::make_unique<T>(...)`](http://en.cppreference.com/w/cpp/memory/unique_ptr/make_unique)
and
@@ -429,7 +192,6 @@ return base::MakeRefCounted<T>(1, 2, 3);
// ...
};
```
-
2. `base::WrapUnique(new Foo)` and `base::WrapUnique(new Foo())` mean something
different if `Foo` does not have a user-defined constructor. Don't make
future maintainers guess whether you left off the '()' on purpose. Use
@@ -448,10 +210,8 @@ See also [TOTW 126](https://abseil.io/tips/126).
## Do not use `auto` to deduce a raw pointer
-The use of the `auto` keyword to deduce the type from the initializing
-expression is encouraged when it improves readability. However, do not use
-`auto` when the type would be deduced to be a pointer type. This can cause
-confusion. Instead, prefer specifying the "pointer" part outside of `auto`:
+Do not use `auto` when the type would be deduced to be a pointer type; this can
+cause confusion. Instead, specify the "pointer" part outside of `auto`:
```cpp
auto item = new Item(); // BAD: auto deduces to Item*, type of |item| is Item*
@@ -460,159 +220,12 @@ auto* item = new Item(); // GOOD: auto deduces to Item, type of |item| is Item*
## Use `const` correctly
-*** promo
-**TLDR:** For safety and simplicity, **don't return pointers or references to
-non-const objects from const methods**. Within that constraint, **mark methods
-as const where possible**. **Avoid `const_cast` to remove const**, except when
+For safety and simplicity, **don't return pointers or references to non-const
+objects from const methods**. Within that constraint, **mark methods as const
+where possible**. **Avoid `const_cast` to remove const**, except when
implementing non-const getters in terms of const getters.
-***
-
-### A brief primer on const
-
-To the compiler, the `const` qualifier on a method refers to _physical
-constness_: calling this method does not change the bits in this object. What
-we want is _logical constness_, which is only partly overlapping: calling this
-method does not affect the object in ways callers will notice, nor does it give
-you a handle with the ability to do so.
-
-Mismatches between these concepts can occur in both directions. When something
-is logically but not physically const, C++ provides the `mutable` keyword to
-silence compiler complaints. This is valuable for e.g. cached calculations,
-where the cache is an implementation detail callers do not care about. When
-something is physically but not logically const, however, the compiler will
-happily accept it, and there are no tools that will automatically save you.
-This discrepancy usually involves pointers. For example,
-
-```cpp
-void T::Cleanup() const { delete pointer_member_; }
-```
-
-Deleting a member is a change callers are likely to care about, so this is
-probably not logically const. But because `delete` does not affect the pointer
-itself, but only the memory it points to, this code is physically const, so it
-will compile.
-
-Or, more subtly, consider this pseudocode from a node in a tree:
-
-```cpp
-class Node {
- public:
- void RemoveSelf() { parent_->RemoveChild(this); }
- void RemoveChild(Node* node) {
- if (node == left_child_)
- left_child_ = nullptr;
- else if (node == right_child_)
- right_child_ = nullptr;
- }
- Node* left_child() const { return left_child_; }
- Node* right_child() const { return right_child_; }
-
- private:
- Node* parent_;
- Node* left_child_;
- Node* right_child_;
-};
-```
-
-The `left_child()` and `right_child()` methods don't change anything about
-`|this|`, so making them `const` seems fine. But they allow code like this:
-
-```cpp
-void SignatureAppearsHarmlessToCallers(const Node& node) {
- node.left_child()->RemoveSelf();
- // Now |node| has no |left_child_|, despite having been passed in by const ref.
-}
-```
-
-The original class definition compiles, and looks locally fine, but it's a
-timebomb: a const method returning a handle that can be used to change the
-system in ways that affect the original object. Eventually, someone will
-actually modify the object, potentially far away from where the handle is
-obtained.
-
-These modifications can be difficult to spot in practice. As we see in the
-previous example, splitting related concepts or state (like "a tree") across
-several objects means a change to one object affects the behavior of others.
-And if this tree is in turn referred to by yet more objects (e.g. the DOM of a
-web page, which influences all sorts of other data structures relating to the
-page), then small changes can have visible ripples across the entire system. In
-a codebase as complex as Chromium, it can be almost impossible to reason about
-what sorts of local changes could ultimately impact the behavior of distant
-objects, and vice versa.
-
-"Logically const correct" code assures readers that const methods will not
-change the system, directly or indirectly, nor allow callers to easily do so.
-They make it easier to reason about large-scale behavior. But since the
-compiler verifies physical constness, it will not guarantee that code is
-actually logically const. Hence the recommendations here.
-
-### Classes of const (in)correctness
-
-In a
-[larger discussion of this issue](https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/C2Szi07dyQo/discussion),
-Matt Giuca
-[postulated three classes of const(in)correctness](https://groups.google.com/a/chromium.org/d/msg/platform-architecture-dev/C2Szi07dyQo/lbHMUQHMAgAJ):
-
-* **Const correct:** All code marked "const" is logically const; all code that
- is logically const is marked "const".
-* **Const okay:** All code marked "const" is logically const, but not all code
- that is logically const is marked "const". (Basically, if you see "const" you
- can trust it, but sometimes it's missing.)
-* **Const broken:** Some code marked "const" is not logically const.
-
-The Chromium codebase currently varies. A significant amount of Blink code is
-"const broken". A great deal of Chromium code is "const okay". A minority of
-code is "const correct".
-
-While "const correct" is ideal, it can take a great deal of work to achieve.
-Const (in)correctness is viral, so fixing one API often requires a yak shave.
-(On the plus side, this same property helps prevent regressions when people
-actually use const objects to access the const APIs.)
-
-At the least, strive to convert code that is "const broken" to be "const okay".
-A simple rule of thumb that will prevent most cases of "const brokenness" is for
-const methods to never return pointers to non-const objects. This is overly
-conservative, but less than you might think, due to how objects can transitively
-affect distant, seemingly-unrelated parts of the system. The discussion thread
-linked above has more detail, but in short, it's hard for readers and reviewers
-to prove that returning pointers-to-non-const is truly safe, and will stay safe
-through later refactorings and modifications. Following this rule is easier
-than debating about whether individual cases are exceptions.
-
-One way to ensure code is "const okay" would be to never mark anything const.
-This is suboptimal for the same reason we don't choose to "never write comments,
-so they can never be wrong". Marking a method "const" provides the reader
-useful information about the system behavior. Also, despite physical constness
-being different than logical constness, using "const" correctly still does catch
-certain classes of errors at compile time. Accordingly, the
-[Google style guide requests the use of const where possible](http://google.github.io/styleguide/cppguide.html#Use_of_const),
-so mark methods const when they are logically const.
-
-Making code more const correct leads to cases where duplicate const and non-const getters are required:
-
-```cpp
-const T* Foo::GetT() const { return t_; }
-T* Foo::GetT() { return t_; }
-```
-
-If the implementation of GetT() is complex, there's a
-[trick to implement the non-const getter in terms of the const one](https://stackoverflow.com/questions/123758/how-do-i-remove-code-duplication-between-similar-const-and-non-const-member-func/123995#123995),
-courtesy of _Effective C++_:
-
-```cpp
-T* Foo::GetT() { return const_cast<T*>(static_cast<const Foo*>(this)->GetT()); }
-```
-
-While this is a mouthful, it does guarantee the implementations won't get out of
-sync and no const-incorrectness will occur. And once you've seen it a few times,
-it's a recognizable pattern.
-
-This is probably the only case where you should see `const_cast` used to remove
-constness. Its use anywhere else is generally indicative of either "const
-broken" code, or a boundary between "const correct" and "const okay" code that
-could change to "const broken" at any future time without warning from the
-compiler. Both cases should be fixed.
+For more information, see [Using Const Correctly](const.md).
## Prefer to use `=default`
@@ -640,23 +253,9 @@ class Good {
Good::Good() = default;
```
-### What are the advantages of `=default`?
-
-* Compiler-defined copy and move operations don't need maintenance every time
- members are added or removed.
-* Compiler-provided special member functions can be "trivial" (if defaulted in
- the class), and can be better optimized by the compiler and library.
-* Types with defaulted constructors can be aggregates (if defaulted in the
- class), and hence support aggregate initialization. User provided constructors
- disqualify a class from being an aggregate.
-* Defaulted functions are constexpr if the implicit version would have been (and
- if defaulted in the class).
-* Using `=default` consistently helps readers identify customized operations.
-
## Comment style
The common ways to represent names in comments are as follows:
-
* Class and type names: `FooClass`
* Function name: `FooFunction()`. The trailing parens disambiguate against
class names, and, occasionally, English words.
diff --git a/chromium/styleguide/c++/c++.md b/chromium/styleguide/c++/c++.md
index 09b10ae077e..6b4aaf11382 100644
--- a/chromium/styleguide/c++/c++.md
+++ b/chromium/styleguide/c++/c++.md
@@ -35,43 +35,13 @@ separate [C++11 use in Chromium](https://chromium-cpp.appspot.com/) page.
with the `ForTesting` suffix. This is checked at presubmit time to ensure
these functions are only called by test files.
- * Test-only constructors cannot have the `ForTesting` suffix. Instead, they
- should be declared protected with a test-only subclass, or private with a
- test-only friend class. They should be commented as `For testing only`.
-
- * Test-only free functions should generally live within a test_support
- target.
-
- * `#if defined(UNIT_TEST)` is problematic and discouraged.
-
## Code formatting
* Put `*` and `&` by the type rather than the variable name.
-
- * When you derive from a base class, group any overriding functions in your
- header file in one labeled section. Use the override specifier on all these
- functions.
-
+ * In class declarations, group function overrides together within each access
+ control section, with one labeled group per parent class.
* Prefer `(foo == 0)` to `(0 == foo)`.
- * Prefer putting delegate classes in their own header files. Implementors of
- the delegate interface will often be included elsewhere, which will often
- cause more coupling with the header of the main class.
-
- * Don't use else after return. So use:
- ```c++
- if (foo)
- return 1;
- return 2;
- ```
- instead of:
- ```c++
- if (foo)
- return 1;
- else
- return 2;
- ```
-
## Unnamed namespaces
Items local to a .cc file should be wrapped in an unnamed namespace. While some
@@ -82,13 +52,10 @@ reduce the size of entry point tables.
## Exporting symbols
-When building shared libraries and DLLs, we need to indicate which functions
-and classes should be visible outside of the library, and which should only be
-visible inside the library.
-
-Symbols can be exported by annotating with a `<COMPONENT>_EXPORT` macro name
-(where `<COMPONENT>` is the name of the component being built, e.g. BASE, NET,
-CONTENT, etc.). Class annotations should precede the class name:
+Symbols can be exported (made visible outside of a shared library/DLL) by
+annotating with a `<COMPONENT>_EXPORT` macro name (where `<COMPONENT>` is the
+name of the component being built, e.g. BASE, NET, CONTENT, etc.). Class
+annotations should precede the class name:
```c++
class FOO_EXPORT Foo {
void Bar();
@@ -102,19 +69,10 @@ Function annotations should precede the return type:
class FooSingleton {
FOO_EXPORT Foo& GetFoo();
FOO_EXPORT Foo& SetFooForTesting(Foo* foo);
- void SetFoo(Foo* foo);
+ void SetFoo(Foo* foo); // Not exported.
};
```
-These examples result in `Foo::Bar()`, `Foo::Baz()`, `FooSingleton::GetFoo()`,
-and `FooSingleton::SetFooForTesting()` all being available outside of the DLL,
-but not `FooSingleton::SetFoo()`.
-
-Whether something is exported is distinct from whether it is public or private,
-or even whether it would normally be considered part of the external API. For
-example, if part of the external API is an inlined function that calls a
-private function, that private function must be exported as well.
-
## Multiple inheritance
Multiple inheritance and virtual inheritance are permitted in Chromium code,
@@ -125,9 +83,7 @@ suffix). Consider whether composition could solve the problem instead.
## Inline functions
Simple accessors should generally be the only inline functions. These should be
-named `unix_hacker_style()`. Virtual functions should never be declared this way.
-For more detail, consult the [C++ Dos and Don'ts](c++-dos-and-donts.md) section
-on inlining.
+named using `snake_case()`. Virtual functions should never be declared this way.
## Logging
@@ -140,10 +96,8 @@ For the rare case when logging needs to stay in the codebase for a while,
prefer `DVLOG(1)` to other logging methods. This avoids bloating the release
executable and in debug can be selectively enabled at runtime by command-line
arguments:
-
- * `--v=n` sets the global log level to n (default 0). All log statements with a
- log level less than or equal to the global level will be printed.
-
+ * `--v=n` sets the global log level to n (default 0). All log statements with
+ a log level less than or equal to the global level will be printed.
* `--vmodule=mod=n[,mod=n,...]` overrides the global log level for the module
mod. Supplying the string foo for mod will affect all files named foo.cc,
while supplying a wildcard like `*bar/baz*` will affect all files with
@@ -178,46 +132,27 @@ Place platform-specific #includes in their own section below the "normal"
## Types
* Use `size_t` for object and allocation sizes, object counts, array and
- pointer offsets, vector indices, and so on. The signed types are
- incorrect and unsafe for these purposes (e.g. integer overflow behavior for
- signed types is undefined in the C and C++ standards, while the behavior is
- defined for unsigned types). The C++ STL and libc are good guides here: they
- use `size_t` and `foo::size_type` for very good reasons.
-
- * Use `size_t` directly in preference to `std::string::size_type` and similar.
-
- * Occasionally classes may have a good reason to use a type other than `size_t`
- for one of these concepts, e.g. as a storage space optimization. In these
- cases, continue to use `size_t` in public-facing function declarations,
- and continue to use unsigned types internally (e.g. `uint32_t`).
-
- * Be aware that `size_t` (object sizes and indices), `off_t` (file offsets),
- `ptrdiff_t` (the difference between two pointer values), `intptr_t` (an
- integer type large enough to hold the value of a pointer), `uint32_t`,
- `uint64_t`, and so on are not necessarily the same. Use the right type for
- your purpose. `CheckedNumeric` is an ergonomic way to perform safe
- arithmetic and casting with and between these different types.
-
+ pointer offsets, vector indices, and so on. This prevents casts when
+ dealing with STL APIs, and if followed consistently across the codebase,
+ minimizes casts elsewhere.
+ * Occasionally classes may have a good reason to use a type other than
+ `size_t` for one of these concepts, e.g. as a storage space optimization. In
+ these cases, continue to use `size_t` in public-facing function
+ declarations, and continue to use unsigned types internally (e.g.
+ `uint32_t`).
* Follow [Google C++ casting
conventions](https://google.github.io/styleguide/cppguide.html#Casting)
to convert arithmetic types when you know the conversion is safe. Use
`checked_cast<T>` (from `base/numerics/safe_conversions.h`) when you need to
`CHECK` that the source value is in range for the destination type. Use
`saturated_cast<T>` if you instead wish to clamp out-of-range values.
-
- * Do not use unsigned types to mean "this value should never be < 0". For
- that, use assertions or run-time checks (as appropriate).
-
- * In cases where the exact size of the type matters (e.g. a 32-bit pixel
- value, a bitmask, or a counter that has to be a particular width), use one
- of the sized types from `<stdint.h>`, e.g. `uint32_t`.
-
+ `CheckedNumeric` is an ergonomic way to perform safe arithmetic and casting
+ in many cases.
* When passing values across network or process boundaries, use
explicitly-sized types for safety, since the sending and receiving ends may
not have been compiled with the same sizes for things like `int` and
`size_t`. However, to the greatest degree possible, avoid letting these
sized types bleed through the APIs of the layers in question.
-
* Don't use `std::wstring`. Use `base::string16` or `base::FilePath` instead.
(Windows-specific code interfacing with system APIs using `wstring` and
`wchar_t` can still use `string16` and `char16`; it is safe to assume that
@@ -228,22 +163,18 @@ Place platform-specific #includes in their own section below the "normal"
When functions need to take raw or smart pointers as parameters, use the
following conventions. Here we refer to the parameter type as `T` and name as
`t`.
-
- * If the function does not modify `t`'s ownership, declare the param as `T*`. The
- caller is expected to ensure `t` stays alive as long as necessary, generally
- through the duration of the call. Exception: In rare cases (e.g. using
- lambdas with STL algorithms over containers of `unique_ptr<>`s), you may be
- forced to declare the param as `const std::unique_ptr<T>&`. Do this only when
- required.
-
+ * If the function does not modify `t`'s ownership, declare the param as `T*`.
+ The caller is expected to ensure `t` stays alive as long as necessary,
+ generally through the duration of the call. Exception: In rare cases (e.g.
+ using lambdas with STL algorithms over containers of `unique_ptr<>`s), you
+ may be forced to declare the param as `const std::unique_ptr<T>&`. Do this
+ only when required.
* If the function takes ownership of a non-refcounted object, declare the
param as `std::unique_ptr<T>`.
-
* If the function (at least sometimes) takes a ref on a refcounted object,
declare the param as `scoped_refptr<T>`. The caller can decide
whether it wishes to transfer ownership (by calling `std::move(t)` when
passing `t`) or retain its ref (by simply passing t directly).
-
* In short, functions should never take ownership of parameters passed as raw
pointers, and there should rarely be a need to pass smart pointers by const
ref.
@@ -277,7 +208,8 @@ forward-declare the type.
## File headers
-All files in Chromium start with a common license header. That header should look like this:
+All files in Chromium start with a common license header. That header should
+look like this:
```c++
// Copyright $YEAR The Chromium Authors. All rights reserved.
@@ -286,18 +218,15 @@ All files in Chromium start with a common license header. That header should loo
```
Some important notes about this header:
-
* There is no `(c)` after `Copyright`.
-
- * `$YEAR` should be set to the current year at the time a file is created, and not changed thereafter.
-
- * For files specific to Chromium OS, replace the word Chromium with the phrase Chromium OS.
-
+ * `$YEAR` should be set to the current year at the time a file is created, and
+ not changed thereafter.
+ * For files specific to Chromium OS, replace the word Chromium with the phrase
+ Chromium OS.
* If the style changes, don't bother to update existing files to comply with
the new style. For the same reason, don't just blindly copy an existing
file's header when creating a new file, since the existing file may use an
outdated style.
-
* The Chromium project hosts mirrors of some upstream open-source projects.
When contributing to these portions of the repository, retain the existing
file headers.
@@ -314,40 +243,37 @@ The `CHECK()` macro will cause an immediate crash if its condition is not met.
(debug builds and some bot configurations, but not end-user builds).
`NOTREACHED()` is equivalent to `DCHECK(false)`. Here are some rules for using
these:
-
* Use `DCHECK()` or `NOTREACHED()` as assertions, e.g. to document pre- and
post-conditions. A `DCHECK()` means "this condition must always be true",
not "this condition is normally true, but perhaps not in exceptional
cases." Things like disk corruption or strange network errors are examples
of exceptional circumstances that nevertheless should not result in
`DCHECK()` failure.
-
* A consequence of this is that you should not handle DCHECK() failures, even
- if failure would result in a crash. Attempting to handle a `DCHECK()` failure
- is a statement that the `DCHECK()` can fail, which contradicts the point of
- writing the `DCHECK()`. In particular, do not write code like the following:
+ if failure would result in a crash. Attempting to handle a `DCHECK()`
+ failure is a statement that the `DCHECK()` can fail, which contradicts the
+ point of writing the `DCHECK()`. In particular, do not write code like the
+ following:
```c++
DCHECK(foo);
- if (!foo) ... // Can't succeed!
+ if (!foo) // Eliminate this code.
+ ...
- if (!bar) {
+ if (!bar) { // Replace this whole conditional with "DCHECK(bar);".
NOTREACHED();
- return; // Replace this whole conditional with "DCHECK(bar);" and keep going instead.
+ return;
}
```
-
* Use `CHECK()` if the consequence of a failed assertion would be a security
vulnerability, where crashing the browser is preferable. Because this takes
down the whole browser, sometimes there are better options than `CHECK()`.
For example, if a renderer sends the browser process a malformed IPC, an
attacker may control the renderer, but we can simply kill the offending
renderer instead of crashing the whole browser.
-
* You can temporarily use `CHECK()` instead of `DCHECK()` when trying to
force crashes in release builds to sniff out which of your assertions is
failing. Don't leave these in the codebase forever; remove them or change
them back once you've solved the problem.
-
* Don't use these macros in tests, as they crash the test binary and leave
bots in a bad state. Use the `ASSERT_xx()` and `EXPECT_xx()` family of
macros, which report failures gracefully and can continue running other
@@ -356,9 +282,7 @@ these:
## Miscellany
* Use UTF-8 file encodings and LF line endings.
-
* Unit tests and performance tests should be placed in the same directory as
the functionality they're testing.
-
* The [C++ Dos and Don'ts](c++-dos-and-donts.md) page has more helpful
information.
diff --git a/chromium/styleguide/c++/c++11.html b/chromium/styleguide/c++/c++11.html
index 2709f845916..3ea1f3ef0ec 100644
--- a/chromium/styleguide/c++/c++11.html
+++ b/chromium/styleguide/c++/c++11.html
@@ -409,6 +409,22 @@ template &lt;typename T&gt;<br/>void Function(T&amp;&amp; t) { ... }</code></td>
</tr>
<tr>
+<td>Lambda capture expressions</td>
+<td><code>auto widget = std::make_unique&lt;Widget&gt;();<br>auto lambda = [widget = std::move(widget)]() {<br>&nbsp;&nbsp;SetWidget(std::move(widget));<br>}</code></td>
+<td>Allows lambda captures to be explicitly initialized with expressions.</td>
+<td><a href="http://en.cppreference.com/w/cpp/language/lambda#Lambda_capture">Lambda capture</a></td>
+<td> Lambda capture expressions should only be used to capture existing names in
+ ways that C++11's direct capture doesn't allow, and should not be used as a
+ way to introduce new names. That is, capture expressions should always shadow
+ existing variables (or data members), and not substantially change their
+ meaning. Usage should be rare.
+ <a href="https://groups.google.com/a/chromium.org/d/msg/cxx/8BHoi4T6ZhA/pWHB6IYoAgAJ">
+ Discussion thread</a> with
+ <a href="https://groups.google.com/a/chromium.org/d/msg/cxx/8BHoi4T6ZhA/WwKXh2RgAgAJ">
+ examples</a></td>
+</tr>
+
+<tr>
<td>Number literal separators</td>
<td><code>float f = 1'000'000.000'1;</code></td>
<td><code>'</code>s anywhere in int or float literals are ignored</td>
@@ -1026,14 +1042,6 @@ something, remove all callers and remove the function instead.</td>
</tr>
<tr>
-<td>Lambda capture expressions</td>
-<td><code>auto widget = std::make_unique&lt;Widget&gt;();<br>auto lambda = [widget = std::move(widget)]() {<br>&nbsp;&nbsp;SetWidget(std::move(widget));<br>}</code></td>
-<td>Allows lambda captures to be explicitly initialized with expressions.</td>
-<td><a href="http://en.cppreference.com/w/cpp/language/lambda#Lambda_capture">Lambda capture</a></td>
-<td>Particularly useful to capture move-only types in a lambda when a reference would go out of scope. Less useful without allowing lambdas to outlive the scope.</td>
-</tr>
-
-<tr>
<td>Variable templates</td>
<td><code>template &lt;typename T&gt;<br>constexpr T tau = T(6.283185307179586476925286766559);</code></td>
<td>Allows templates that declare variables, rather than functions or classes.</td>
diff --git a/chromium/styleguide/c++/const.md b/chromium/styleguide/c++/const.md
new file mode 100644
index 00000000000..7b53f8289af
--- /dev/null
+++ b/chromium/styleguide/c++/const.md
@@ -0,0 +1,155 @@
+# Using Const Correctly
+
+The **TLDR**, as stated in [C++ Dos and Don'ts](c++-dos-and-donts.md):
+*** promo
+For safety and simplicity, **don't return pointers or references to non-const
+objects from const methods**. Within that constraint, **mark methods as const
+where possible**. **Avoid `const_cast` to remove const**, except when
+implementing non-const getters in terms of const getters.
+***
+
+## A brief primer on const
+
+To the compiler, the `const` qualifier on a method refers to _physical
+constness_: calling this method does not change the bits in this object. What
+we want is _logical constness_, which is only partly overlapping: calling this
+method does not affect the object in ways callers will notice, nor does it give
+you a handle with the ability to do so.
+
+Mismatches between these concepts can occur in both directions. When something
+is logically but not physically const, C++ provides the `mutable` keyword to
+silence compiler complaints. This is valuable for e.g. cached calculations,
+where the cache is an implementation detail callers do not care about. When
+something is physically but not logically const, however, the compiler will
+happily accept it, and there are no tools that will automatically save you.
+This discrepancy usually involves pointers. For example,
+
+```cpp
+void T::Cleanup() const { delete pointer_member_; }
+```
+
+Deleting a member is a change callers are likely to care about, so this is
+probably not logically const. But because `delete` does not affect the pointer
+itself, but only the memory it points to, this code is physically const, so it
+will compile.
+
+Or, more subtly, consider this pseudocode from a node in a tree:
+
+```cpp
+class Node {
+ public:
+ void RemoveSelf() { parent_->RemoveChild(this); }
+ void RemoveChild(Node* node) {
+ if (node == left_child_)
+ left_child_ = nullptr;
+ else if (node == right_child_)
+ right_child_ = nullptr;
+ }
+ Node* left_child() const { return left_child_; }
+ Node* right_child() const { return right_child_; }
+
+ private:
+ Node* parent_;
+ Node* left_child_;
+ Node* right_child_;
+};
+```
+
+The `left_child()` and `right_child()` methods don't change anything about
+`|this|`, so making them `const` seems fine. But they allow code like this:
+
+```cpp
+void SignatureAppearsHarmlessToCallers(const Node& node) {
+ node.left_child()->RemoveSelf();
+ // Now |node| has no |left_child_|, despite having been passed in by const ref.
+}
+```
+
+The original class definition compiles, and looks locally fine, but it's a
+timebomb: a const method returning a handle that can be used to change the
+system in ways that affect the original object. Eventually, someone will
+actually modify the object, potentially far away from where the handle is
+obtained.
+
+These modifications can be difficult to spot in practice. As we see in the
+previous example, splitting related concepts or state (like "a tree") across
+several objects means a change to one object affects the behavior of others.
+And if this tree is in turn referred to by yet more objects (e.g. the DOM of a
+web page, which influences all sorts of other data structures relating to the
+page), then small changes can have visible ripples across the entire system. In
+a codebase as complex as Chromium, it can be almost impossible to reason about
+what sorts of local changes could ultimately impact the behavior of distant
+objects, and vice versa.
+
+"Logically const correct" code assures readers that const methods will not
+change the system, directly or indirectly, nor allow callers to easily do so.
+They make it easier to reason about large-scale behavior. But since the
+compiler verifies physical constness, it will not guarantee that code is
+actually logically const. Hence the recommendations here.
+
+## Classes of const (in)correctness
+
+In a
+[larger discussion of this issue](https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/C2Szi07dyQo/discussion),
+Matt Giuca
+[postulated three classes of const(in)correctness](https://groups.google.com/a/chromium.org/d/msg/platform-architecture-dev/C2Szi07dyQo/lbHMUQHMAgAJ):
+
+* **Const correct:** All code marked "const" is logically const; all code that
+ is logically const is marked "const".
+* **Const okay:** All code marked "const" is logically const, but not all code
+ that is logically const is marked "const". (Basically, if you see "const" you
+ can trust it, but sometimes it's missing.)
+* **Const broken:** Some code marked "const" is not logically const.
+
+The Chromium codebase currently varies. A significant amount of Blink code is
+"const broken". A great deal of Chromium code is "const okay". A minority of
+code is "const correct".
+
+While "const correct" is ideal, it can take a great deal of work to achieve.
+Const (in)correctness is viral, so fixing one API often requires a yak shave.
+(On the plus side, this same property helps prevent regressions when people
+actually use const objects to access the const APIs.)
+
+At the least, strive to convert code that is "const broken" to be "const okay".
+A simple rule of thumb that will prevent most cases of "const brokenness" is for
+const methods to never return pointers to non-const objects. This is overly
+conservative, but less than you might think, due to how objects can transitively
+affect distant, seemingly-unrelated parts of the system. The discussion thread
+linked above has more detail, but in short, it's hard for readers and reviewers
+to prove that returning pointers-to-non-const is truly safe, and will stay safe
+through later refactorings and modifications. Following this rule is easier
+than debating about whether individual cases are exceptions.
+
+One way to ensure code is "const okay" would be to never mark anything const.
+This is suboptimal for the same reason we don't choose to "never write comments,
+so they can never be wrong". Marking a method "const" provides the reader
+useful information about the system behavior. Also, despite physical constness
+being different than logical constness, using "const" correctly still does catch
+certain classes of errors at compile time. Accordingly, the
+[Google style guide requests the use of const where possible](http://google.github.io/styleguide/cppguide.html#Use_of_const),
+so mark methods const when they are logically const.
+
+Making code more const correct leads to cases where duplicate const and non-const getters are required:
+
+```cpp
+const T* Foo::GetT() const { return t_; }
+T* Foo::GetT() { return t_; }
+```
+
+If the implementation of GetT() is complex, there's a
+[trick to implement the non-const getter in terms of the const one](https://stackoverflow.com/questions/123758/how-do-i-remove-code-duplication-between-similar-const-and-non-const-member-func/123995#123995),
+courtesy of _Effective C++_:
+
+```cpp
+T* Foo::GetT() { return const_cast<T*>(static_cast<const Foo*>(this)->GetT()); }
+```
+
+While this is a mouthful, it does guarantee the implementations won't get out of
+sync and no const-incorrectness will occur. And once you've seen it a few times,
+it's a recognizable pattern.
+
+This is probably the only case where you should see `const_cast` used to remove
+constness. Its use anywhere else is generally indicative of either "const
+broken" code, or a boundary between "const correct" and "const okay" code that
+could change to "const broken" at any future time without warning from the
+compiler. Both cases should be fixed.