0

Improve harmonization of Blink/Chromium/Google style guides/docs.

Major changes:
* Remove mutable reference section from Blink guide due to recent
  Google style changes.

Minor changes:
* Remove Blink guide section on documentation since the Google style
  guide already mandates this.
* Add section on TODO formatting to Dos-And-Don'ts page; remove
  similar section from Blink guide in hopes the Google guidelines +
  Dos-And-Dont's text covers it.
* Add information about C++17 (lack of) support.

Trivial changes:
* Clarify that the Blink style guide is a diff atop the Chromium
  guide (which is a diff atop the Google guide).
* Cite Chromium guidance around bare "new" from Blink guide.
* Minor link fixes, wording changes, inlining.

Bug: none
Change-Id: I1e3c2b265930cc73501e3557b25ca52b6902126e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2223452
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Auto-Submit: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#774352}
This commit is contained in:
Peter Kasting
2020-06-02 21:44:40 +00:00
committed by Commit Bot
parent 750bcab047
commit 777e130e2f
4 changed files with 39 additions and 80 deletions

@ -1,29 +1,14 @@
# Blink C++ Style Guide
This document is a list of differences from the overall [Chromium Style Guide],
which is in turn a set of differences from the [Google C++ Style Guide]. The
This document is a list of differences from the overall
[Chromium Style Guide](c++.md), which is in turn a set of differences from the
[Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html). The
long-term goal is to make both Chromium and Blink style more similar to Google
style over time, so this document aims to highlight areas where Blink style
differs from Google style.
differs from Chromium style.
[TOC]
## May use mutable reference arguments
Mutable reference arguments are permitted in Blink, in contrast to Google style.
> Note: This rule is under [discussion](https://groups.google.com/a/chromium.org/d/msg/blink-dev/O7R4YwyPIHc/mJyEyJs-EAAJ).
**OK:**
```c++
// 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)) {
// ...
}
```
## Prefer WTF types over STL and base types
See [Blink readme](../../third_party/blink/renderer/README.md#Type-dependencies)
@ -47,11 +32,11 @@ When interacting with WTF types, use `wtf_size_t` instead of `size_t`.
## Do not use `new` and `delete`
Object lifetime should not be managed using raw `new` and `delete`. Prefer to
allocate objects instead using `std::make_unique`, `base::MakeRefCounted` or
`blink::MakeGarbageCollected`, depending on the type, and manage their lifetime
using appropriate smart pointers and handles (`std::unique_ptr`, `scoped_refptr`
and strong Blink GC references, respectively). See [How Blink Works](https://docs.google.com/document/d/1aitSOucL0VHZa9Z2vbRJSyAIsAz24kX8LFByQ5xQnUg/edit#heading=h.ekwf97my4bgf)
Chromium [recommends avoiding bare new/delete](c++-dos-and-donts.md#use-and-instead-of-bare);
Blink bans them. In addition to the options there, Blink objects may be
allocated using `blink::MakeGarbageCollected` and manage their lifetimes using
strong Blink GC references, depending on the type. See
[How Blink Works](https://docs.google.com/document/d/1aitSOucL0VHZa9Z2vbRJSyAIsAz24kX8LFByQ5xQnUg/edit#heading=h.ekwf97my4bgf)
for more information.
## Don't mix Create() factory methods and public constructors in one class.
@ -210,11 +195,13 @@ class RootInlineBox {
```
### May leave obvious parameter names out of function declarations
[Google C++ Style Guide] allows us to leave parameter names out only if
the parameter is not used. In Blink, you may leave obvious parameter
names out of function declarations for historical reason. A good rule of
thumb is if the parameter type name contains the parameter name (without
trailing numbers or pluralization), then the parameter name isnt needed.
The
[Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions)
allows omitting parameter names out when they are unused. In Blink, you may
leave obvious parameter names out of function declarations for historical
reasons. A good rule of thumb is if the parameter type name contains the
parameter name (without trailing numbers or pluralization), then the parameter
name isnt needed.
**Good:**
```c++
@ -324,45 +311,7 @@ if (frame_->Loader().ShouldClose(false)) {
// ...
```
## Comments
Please follow the standard [Chromium Documentation Guidelines]. In particular,
most classes should have a class-level comment describing the purpose, while
non-trivial code should have comments describing why the code is written the
way it is. Often, what is apparent when the code is written is not so obvious
a year later.
From [Google C++ Style Guide: Comments]:
> Giving sensible names to types and variables is much better than obscure
> names that must be explained through comments.
### Use `README.md` to document high-level components
Documentation for a related-set of classes and how they interact should be done
with a `README.md` file in the root directory of a component.
### TODO style
Comments for future work should use `TODO` and have a name or bug attached.
From [Google C++ Style Guide: TODO Comments]:
> The person named is not necessarily the person who must fix it.
**Good:**
```c++
// TODO(dcheng): Clean up after the Blink rename is done.
// TODO(https://crbug.com/675877): Clean up after the Blink rename is done.
```
**Bad:**
```c++
// FIXME(dcheng): Clean up after the Blink rename is done.
// FIXME: Clean up after the Blink rename is done.
// TODO: Clean up after the Blink rename is done.
```
[Chromium Style Guide]: c++.md
[Google C++ Style Guide]: https://google.github.io/styleguide/cppguide.html
[Chromium Documentation Guidelines]: ../../docs/documentation_guidelines.md
[Google C++ Style Guide: Comments]: https://google.github.io/styleguide/cppguide.html#Comments
[Google C++ Style Guide: TODO Comments]:https://google.github.io/styleguide/cppguide.html#TODO_Comments

@ -2,8 +2,8 @@
## A Note About Usage
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
Unlike the [style guide](c++.md), 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
@ -270,15 +270,20 @@ Good::Good() = default;
The common ways to represent names in comments are as follows:
* Class and type names: `FooClass`
* Function name: `FooFunction()`. The trailing parens disambiguate against
* Function names: `FooFunction()`. The trailing parens disambiguate against
class names, and, occasionally, English words.
* Variable name: `|foo_var|`. Again, the vertical lines disambiguate against
* Variable names: `|foo_var|`. Again, the vertical lines disambiguate against
English words, and, occasionally, inlined function names. Code search will
also automatically convert `|foo_var|` into a clickable link.
* Tracking comments for future improvements: `// TODO(crbug.com/12345): ...`,
or, less optimally, `// TODO(knowledgeable_username): ...`. Tracking bugs
provide space to give background context and current status; a username might
at least provide a starting point for asking about an issue.
```cpp
// FooImpl implements the FooBase class.
// FooFunction() modifies |foo_member_|.
// TODO(crbug.com/1): Rename things to something more descriptive than "foo".
```
## Named namespaces

@ -1,6 +1,7 @@
# Chromium C++ style guide
_For other languages, please see the [Chromium style guides](https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md)._
_For other languages, please see the
[Chromium style guides](https://chromium.googlesource.com/chromium/src/+/master/styleguide/styleguide.md)._
Chromium follows the [Google C++ Style
Guide](https://google.github.io/styleguide/cppguide.html) unless an exception
@ -20,11 +21,14 @@ Blink code in `third_party/blink` uses [Blink style](blink-c++.md).
## Modern C++ features
Some features of C++ remain forbidden, even as Chromium adopts newer versions
of the C++ language and standard library. These should be similar to those
allowed in Google style, but may occasionally differ. The status of modern C++
features in Chromium is tracked in the separate
[C++ use in Chromium](https://chromium-cpp.appspot.com/) page.
Google style
[targets C++17](https://google.github.io/styleguide/cppguide.html#C++_Version).
Chromium targets C++14; [C++17 support](https://crbug.com/752720) is not
expected before
[mid-2021](https://blog.chromium.org/2020/01/moving-forward-from-chrome-apps.html).
Additionally, some features of supported C++ versions remain forbidden. The
status of Chromium's C++ support is covered in more detail in
[Modern C++ use in Chromium](https://chromium-cpp.appspot.com/).
## Naming
@ -34,7 +38,8 @@ features in Chromium is tracked in the separate
## Test-only Code
* Functions used only for testing should be restricted to test-only usages
with the testing suffixes supported by [PRESUMBIT.py](https://chromium.googlesource.com/chromium/src/+/master/PRESUBMIT.py).
with the testing suffixes supported by
[PRESUMBIT.py](https://chromium.googlesource.com/chromium/src/+/master/PRESUBMIT.py).
`ForTesting` is the conventional suffix although similar patterns, such as
`ForTest`, are also accepted. These suffixes are checked at presubmit time
to ensure the functions are called only by test files.

@ -18,7 +18,7 @@ table tbody tr td:first-child {
</head>
<body>
<div id="content">
<h1>C++ use in Chromium</h1>
<h1>Modern C++ use in Chromium</h1>
<p><i>This document lives at src/styleguide/c++/c++11.html in a Chromium
checkout and is part of the more general
@ -47,7 +47,7 @@ default-allow all non-banned portions of the standard. The current status of
existing standards is:
<ul><li><b>C++11:</b> <i>Default allowed; see banned features below</i></li>
<li><b>C++14:</b> <i>Default allowed; see banned features below</i></li>
<li><b>C++17:</b> <i>Not yet supported in Chromium</i></li>
<li><b>C++17:</b> <i>Not yet supported in Chromium, unlikely before mid-2021; <a href="http://crbug.com/752720">tracking bug</a></i></li>
<li><b>C++20:</b> <i>Not yet standardized</i></li></ul></p>
<h2>Table of Contents</h2>