|
|
|
|
# Open Screen Library Style Guide
|
|
|
|
|
|
|
|
|
|
The Open Screen Library follows the [Chromium C++ coding style](https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md)
|
|
|
|
|
which, in turn, defers to the [Google C++ Style Guide](https://google.github.io/styleguide/cppguide.html).
|
|
|
|
|
We also follow the [Chromium C++ Do's and Don'ts](https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-and-donts).
|
|
|
|
|
|
|
|
|
|
C++14 language and library features are allowed in the Open Screen Library
|
|
|
|
|
according to the [C++14 use in Chromium](https://chromium-cpp.appspot.com)
|
|
|
|
|
guidelines.
|
|
|
|
|
|
|
|
|
|
In general Open Screen follows [You Aren't Gonna Need
|
|
|
|
|
It](https://martinfowler.com/bliki/Yagni.html) principles.
|
|
|
|
|
|
|
|
|
|
## Disallowed Styles and Features
|
|
|
|
|
|
|
|
|
|
Blink style is *not allowed* anywhere in the Open Screen Library.
|
|
|
|
|
|
|
|
|
|
C++17-only features are currently *not allowed* in the Open Screen Library.
|
|
|
|
|
|
|
|
|
|
GCC does not support designated initializers for non-trivial types. This means
|
|
|
|
|
that the `.member = value` struct initialization syntax is not supported unless
|
|
|
|
|
all struct members are primitive types or structs of primitive types (i.e. no
|
|
|
|
|
unions, complex constructors, etc.).
|
|
|
|
|
|
|
|
|
|
## Modifications to the Chromium C++ Guidelines
|
|
|
|
|
|
|
|
|
|
- `<functional>` and `std::function` objects are allowed.
|
|
|
|
|
- `<chrono>` is allowed and encouraged for representation of time.
|
|
|
|
|
- Abseil types are allowed based on the allowed list in [DEPS](
|
|
|
|
|
https://chromium.googlesource.com/openscreen/+/refs/heads/master/DEPS).
|
|
|
|
|
- However, Abseil types **must not be used in public APIs**.
|
|
|
|
|
- `<thread>` and `<mutex>` are allowed, but discouraged from general use as the
|
|
|
|
|
library only needs to handle threading in very specific places;
|
|
|
|
|
see [threading.md](threading.md).
|
|
|
|
|
- Following YAGNI principles, only implement comparison operator overloads as
|
|
|
|
|
needed; for example, implementing operator< for use in an STL container does
|
|
|
|
|
not require implementing all comparison operators.
|
|
|
|
|
|
|
|
|
|
## Code Syntax
|
|
|
|
|
|
|
|
|
|
- Braces are optional for single-line if statements; follow the style of
|
|
|
|
|
surrounding code.
|
|
|
|
|
- Using-declarations are banned from headers. Type aliases should not be
|
|
|
|
|
included in headers, except at class scope when they form part of the class
|
|
|
|
|
definition.
|
|
|
|
|
- Exception: Using-declarations for convenience may be put into a shared
|
|
|
|
|
header for internal library use. These may only be included in
|
|
|
|
|
.cc files.
|
|
|
|
|
- Exception: if a class has an associated POD identifier (int/string), then
|
|
|
|
|
declare a type alias at namespace scope for that identifier instead of using
|
|
|
|
|
the POD type. For example, if a class Foo has a string identifier, declare
|
|
|
|
|
`using FooId = std::string` in foo.h.
|
|
|
|
|
|
|
|
|
|
## Copy and Move Operators
|
|
|
|
|
|
|
|
|
|
Use the following guidelines when deciding on copy and move semantics for
|
|
|
|
|
objects:
|
|
|
|
|
|
|
|
|
|
- Objects with data members greater than 32 bytes should be move-able.
|
|
|
|
|
- Known large objects (I/O buffers, etc.) should be be move-only.
|
|
|
|
|
- Variable length objects should be move-able
|
|
|
|
|
(since they may be arbitrarily large in size) and, if possible, move-only.
|
|
|
|
|
- Inherently non-copyable objects (like sockets) should be move-only.
|
|
|
|
|
|
|
|
|
|
### Default Copy and Move Operators
|
|
|
|
|
|
|
|
|
|
We [prefer the use of `default` and `delete`](https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-and-donts#TOC-Prefer-to-use-default)
|
|
|
|
|
to declare the copy and move semantics of objects. See
|
|
|
|
|
[Stroustrup's C++ FAQ](http://www.stroustrup.com/C++11FAQ.html#default)
|
|
|
|
|
for details on how to do that.
|
|
|
|
|
|
|
|
|
|
Classes should prefer [member
|
|
|
|
|
initialization](https://en.cppreference.com/w/cpp/language/data_members#Member_initialization)
|
|
|
|
|
for POD members (as opposed to value initialization in the constructor). Every POD
|
|
|
|
|
member must be initialized by every constructor, of course, to prevent
|
|
|
|
|
(https://en.cppreference.com/w/cpp/language/default_initialization)[default
|
|
|
|
|
initialization] from setting them to indeterminate values.
|
|
|
|
|
|
|
|
|
|
### User Defined Copy and Move Operators
|
|
|
|
|
|
|
|
|
|
Classes should follow the [rule of three/five/zero](https://en.cppreference.com/w/cpp/language/rule_of_three).
|
|
|
|
|
|
|
|
|
|
This means that if they implement a destructor or any of the copy or move
|
|
|
|
|
operators, then all five (destructor, copy & move constructors, copy & move
|
|
|
|
|
assignment operators) should be defined or marked as `delete`d as appropriate.
|
|
|
|
|
Finally, polymorphic base classes with virtual destructors should `default` all
|
|
|
|
|
constructors, destructors, and assignment operators.
|
|
|
|
|
|
|
|
|
|
Note that operator definitions belong in the source (`.cc`) file, including
|
|
|
|
|
`default`, with the exception of `delete`, because it is not a definition,
|
|
|
|
|
rather a declaration that there is no definition, and thus belongs in the header
|
|
|
|
|
(`.h`) file.
|
|
|
|
|
|
|
|
|
|
## Passing Objects by Value or Reference
|
|
|
|
|
|
|
|
|
|
In most cases, pass by value is preferred as it is simpler and more flexible.
|
|
|
|
|
If the object being passed is move-only, then no extra copies will be made. If
|
|
|
|
|
it is not, be aware this may result in unintended copies.
|
|
|
|
|
|
|
|
|
|
To guarantee that ownership is transferred, pass by rvalue reference for objects
|
|
|
|
|
with move operators. Often this means adding an overload that takes a const
|
|
|
|
|
reference as well.
|
|
|
|
|
|
|
|
|
|
Pass ownership via `std::unique_ptr<>` for non-movable objects.
|
|
|
|
|
|
|
|
|
|
Ref: [Google Style Guide on Rvalue References](https://google.github.io/styleguide/cppguide.html#Rvalue_references)
|
|
|
|
|
|
|
|
|
|
## Noexcept
|
|
|
|
|
|
|
|
|
|
We prefer to use `noexcept` on move constructors. Although exceptions are not
|
|
|
|
|
allowed, this declaration [enables STL optimizations](https://en.cppreference.com/w/cpp/language/noexcept_spec).
|
|
|
|
|
|
|
|
|
|
TODO(https://issuetracker.google.com/issues/160731444): Enforce this
|
|
|
|
|
|
|
|
|
|
Additionally, GCC requires that any type using a defaulted `noexcept` move
|
|
|
|
|
constructor/operator= has a `noexcept` copy or move constructor/operator= for
|
|
|
|
|
all of its members.
|
|
|
|
|
|
|
|
|
|
## Template Programming
|
|
|
|
|
|
|
|
|
|
Template programming should be not be used to write generic algorithms or
|
|
|
|
|
classes when there is no application of the code to more than one type. When
|
|
|
|
|
similar code applies to multiple types, use templates sparingly and on a
|
|
|
|
|
case-by-case basis.
|
|
|
|
|
|
|
|
|
|
## Unit testing
|
|
|
|
|
|
|
|
|
|
Follow Google’s testing best practices for C++. Design classes in such a way
|
|
|
|
|
that testing the public API is sufficient. Strive to follow this guidance,
|
|
|
|
|
trading off with the amount of public API surfaces needed and long-term
|
|
|
|
|
maintainability.
|
|
|
|
|
|
|
|
|
|
Ref: [Test Behavior, Not Implementation](https://testing.googleblog.com/2013/08/testing-on-toilet-test-behavior-not.html)
|
|
|
|
|
|
|
|
|
|
## Open Screen Library Features
|
|
|
|
|
|
|
|
|
|
- For public API functions that return values or errors, please return
|
|
|
|
|
[`ErrorOr<T>`](https://chromium.googlesource.com/openscreen/+/master/platform/base/error.h).
|
|
|
|
|
- In the implementation of public APIs invoked by the embedder, use
|
|
|
|
|
`OSP_DCHECK(TaskRunner::IsRunningOnTaskRunner())` to catch thread safety
|
|
|
|
|
problems early.
|
|
|
|
|
|
|
|
|
|
### Helpers for `std::chrono`
|
|
|
|
|
|
|
|
|
|
One of the trickier parts of the Open Screen Library is using time and clock
|
|
|
|
|
functionality provided by [`platform/api/time.h`](https://chromium.googlesource.com/openscreen/+/refs/heads/master/platform/api/time.h).
|
|
|
|
|
|
|
|
|
|
- When working extensively with `std::chrono` types in implementation code,
|
|
|
|
|
[`util/chrono_helpers.h`](https://chromium.googlesource.com/openscreen/+/refs/heads/master/util/chrono_helpers.h)
|
|
|
|
|
can be included for access to type aliases for
|
|
|
|
|
common `std::chrono` types, so they can just be referred to as `hours`,
|
|
|
|
|
`milliseconds`, etc. This header also includes helpful conversion functions,
|
|
|
|
|
such as `to_milliseconds` instead of
|
|
|
|
|
`std::chrono::duration_cast<std::chrono::milliseconds>`.
|
|
|
|
|
`util/chrono_helpers.h` can only be used in library-internal code, and
|
|
|
|
|
this is enforced by DEPS.
|
|
|
|
|
- `Clock::duration` is defined currently as `std::chrono::microseconds`, and
|
|
|
|
|
thus is generally not suitable as a time type (developers generally think in
|
|
|
|
|
milliseconds). Prefer casting from explicit time types using
|
|
|
|
|
`Clock::to_duration`, e.g. `Clock::to_duration(seconds(2))`
|
|
|
|
|
instead of using `Clock::duration` types directly.
|
|
|
|
|
|
|
|
|
|
### OSP_CHECK and OSP_DCHECK
|
|
|
|
|
|
|
|
|
|
These are provided in [`platform/api/logging.h`](https://chromium.googlesource.com/openscreen/+/refs/heads/master/platform/api/logging.h)
|
|
|
|
|
and act as run-time assertions (i.e., they
|
|
|
|
|
test an expression, and crash the program if it evaluates as false). They are
|
|
|
|
|
not only useful in determining correctness, but also serve as inline
|
|
|
|
|
documentation of the assumptions being made in the code. They should be used in
|
|
|
|
|
cases where they would fail only due to current or future coding errors.
|
|
|
|
|
|
|
|
|
|
These should *not* be used to sanitize non-const data, or data otherwise derived
|
|
|
|
|
from external inputs. Instead, one should code proper error-checking and
|
|
|
|
|
handling for such things.
|
|
|
|
|
|
|
|
|
|
OSP_CHECKs are "turned on" for all build types. However, OSP_DCHECKs are only
|
|
|
|
|
"turned on" in Debug builds, or in any build where the `dcheck_always_on=true`
|
|
|
|
|
GN argument is being used. In fact, at any time during development (including
|
|
|
|
|
Release builds), it is highly recommended to use `dcheck_always_on=true` to
|
|
|
|
|
catch bugs.
|
|
|
|
|
|
|
|
|
|
When OSP_DCHECKs are "turned off" they effectively become code comments: All
|
|
|
|
|
supported compilers will not generate any code, and they will automatically
|
|
|
|
|
strip-out unused functions and constants referenced in OSP_DCHECK expressions
|
|
|
|
|
(unless they are "extern" to the local module); and so there is absolutely no
|
|
|
|
|
run-time/space overhead when the program runs. For this reason, a developer need
|
|
|
|
|
not explicitly sprinkle "#if OSP_DCHECK_IS_ON()" guards all around any
|
|
|
|
|
functions, variables, etc. that will be unused in "DCHECK off" builds.
|
|
|
|
|
|
|
|
|
|
Use OSP_DCHECK and OSP_CHECK in accordance with the
|
|
|
|
|
[Chromium guidance for DCHECK/CHECK](https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#check_dcheck_and-notreached).
|