Skip to content

Latest commit

 

History

History
227 lines (175 loc) · 10.7 KB

style_guide.md

File metadata and controls

227 lines (175 loc) · 10.7 KB

Open Screen Library Style Guide

The Open Screen Library follows the Chromium C++ coding style which, in turn, defers to the Google C++ Style Guide. We also follow the Chromium C++ Do's and Don'ts.

C++17 is the maximum language level allowed in the library. For C++11, C++14, and C++17 language and library features, we follow C++ features in Chromium guidelines with the following exceptions noted below.

In general Open Screen follows You Aren't Gonna Need It principles. This means we avoid adding generic classes, functions, or libraries that are only used in one place for one thing. Once there are multiple usages of a function that can be handled in a generic way, that function is a candidate for our base libraries in util/ and platform/base.

Disallowed Styles and Features

Blink style is not allowed anywhere in the Open Screen Library.

C++20-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.
  • However, Abseil types must not be used in public APIs. Public APIs are headers in targets with GN visibility outside the library or in public/ folders.
  • <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.
  • 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.
    • Case by case exceptions may be made for readability/convenience, e.g. as in platform/base/span.h.

Initialization

Generally, follow the guidance in Abseil ToTw #88. This means to prefer prefer copy-initialization over direct-initialization when available, and use parentheses over curly braces when resorting to direct-initialization.

Preferred:

int x = 2;
std::string foo = "Hello World";
std::vector<int> v = {1, 2, 3};
auto matrix = std::make_unique<NewMatrix>(rows, cols);
MyStruct x = {true, 5.0};

Discouraged:

int x{2};
std::string foo{"Hello World"};
std::vector<int> v{1, 2, 3};
std::unique_ptr<Matrix> matrix{NewMatrix(rows, cols)};
MyStruct x{true, 5.0};

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 to declare the copy and move semantics of objects. See Stroustrup's C++ FAQ for details on how to do that.

Classes should prefer 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.

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 deleted 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

Noexcept

We prefer to use noexcept on move constructors. Although exceptions are not allowed, this declaration enables STL optimizations.

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 usage of the algorithm or class with more than one dependent type. When similar code applies to multiple types, use templates sparingly and on a case-by-case basis. Keep template metaprogramming (i.e., conditional and/or computed template parameters) to a minimum to ensure type safety.

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

Open Screen Library Features

  • For public API functions that return values or errors, please return ErrorOr<T>.
  • 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.

  • When working extensively with std::chrono types in implementation code, 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 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.