ES: Expressions and statements

Expressions and statements are the lowest and most direct way of expressing actions and computation. Declarations in local scopes are statements.

For naming, commenting, and indentation rules, see (NL: Naming and layout).

General rules:

Declaration rules:

Expression rules:

Statement rules:

Arithmetic rules:

ES.1: Prefer the standard library to other libraries and to "handcrafted code"

Reason

Code using a library can be much easier to write than code working directly with language features, much shorter, tend to be of a higher level of abstraction, and the library code is presumably already tested. The ISO C++ Standard Library is among the most widely known and best tested libraries. It is available as part of all C++ implementations.

Example
auto sum = accumulate(begin(a), end(a), 0.0);   // good

a range version of accumulate would be even better:

auto sum = accumulate(v, 0.0); // better

but don't hand-code a well-known algorithm:

int max = v.size();   // bad: verbose, purpose unstated
double sum = 0.0;
for (int i = 0; i < max; ++i)
    sum = sum + v[i];
Exception

Large parts of the standard library rely on dynamic allocation (free store). These parts, notably the containers but not the algorithms, are unsuitable for some hard-real-time and embedded applications. In such cases, consider providing/using similar facilities, e.g., a standard-library-style container implemented using a pool allocator.

Enforcement

Not easy. ??? Look for messy loops, nested loops, long functions, absence of function calls, lack of use of non-built-in types. Cyclomatic complexity?

ES.2: Prefer suitable abstractions to direct use of language features

Reason

A "suitable abstraction" (e.g., library or class) is closer to the application concepts than the bare language, leads to shorter and clearer code, and is likely to be better tested.

Example
vector<string> read1(istream& is)   // good
{
    vector<string> res;
    for (string s; is >> s;)
        res.push_back(s);
    return res;
}

The more traditional and lower-level near-equivalent is longer, messier, harder to get right, and most likely slower:

char** read2(istream& is, int maxelem, int maxstring, int* nread)   // bad: verbose and incomplete
{
    auto res = new char*[maxelem];
    int elemcount = 0;
    while (is && elemcount < maxelem) {
        auto s = new char[maxstring];
        is.read(s, maxstring);
        res[elemcount++] = s;
    }
    nread = &elemcount;
    return res;
}

Once the checking for overflow and error handling has been added that code gets quite messy, and there is the problem remembering to delete the returned pointer and the C-style strings that array contains.

Enforcement

Not easy. ??? Look for messy loops, nested loops, long functions, absence of function calls, lack of use of non-built-in types. Cyclomatic complexity?

ES.3: Don't repeat yourself, avoid redundant code

Duplicated or otherwise redundant code obscures intent, makes it harder to understand the logic, and makes maintenance harder, among other problems. It often arises from cut-and-paste programming.

Use standard algorithms where appropriate, instead of writing some own implementation.

See also: ((SL.1), ES.11)

Example
void func(bool flag)    // Bad, duplicated code.
{
    if (flag) {
        x();
        y();
    }
    else {
        x();
        z();
    }
}
void func(bool flag)    // Better, no duplicated code.
{
    x();
    if (flag)
        y();
    else
        z();
}
Enforcement
  • Use a static analyzer. It will catch at least some redundant constructs.
  • Code review

ES.dcl: Declarations

A declaration is a statement. A declaration introduces a name into a scope and might cause the construction of a named object.

ES.5: Keep scopes small

Reason

Readability. Minimize resource retention. Avoid accidental misuse of value.

Alternative formulation: Don't declare a name in an unnecessarily large scope.

Example
void use()
{
    int i;    // bad: i is needlessly accessible after loop
    for (i = 0; i < 20; ++i) { /* ... */ }
    // no intended use of i here
    for (int i = 0; i < 20; ++i) { /* ... */ }  // good: i is local to for-loop
    if (auto pc = dynamic_cast<Circle*>(ps)) {  // good: pc is local to if-statement
        // ... deal with Circle ...
    }
    else {
        // ... handle error ...
    }
}
Example, bad
void use(const string& name)
{
    string fn = name + ".txt";
    ifstream is {fn};
    Record r;
    is >> r;
    // ... 200 lines of code without intended use of fn or is ...
}

This function is by most measures too long anyway, but the point is that the resources used by fn and the file handle held by is are retained for much longer than needed and that unanticipated use of is and fn could happen later in the function. In this case, it might be a good idea to factor out the read:

Record load_record(const string& name)
{
    string fn = name + ".txt";
    ifstream is {fn};
    Record r;
    is >> r;
    return r;
}
void use(const string& name)
{
    Record r = load_record(name);
    // ... 200 lines of code ...
}
Enforcement
  • Flag loop variable declared outside a loop and not used after the loop
  • Flag when expensive resources, such as file handles and locks are not used for N-lines (for some suitable N)

ES.6: Declare names in for-statement initializers and conditions to limit scope

Reason

Readability. Limit the loop variable visibility to the scope of the loop. Avoid using the loop variable for other purposes after the loop. Minimize resource retention.

Example
void use()
{
    for (string s; cin >> s;)
        v.push_back(s);
    for (int i = 0; i < 20; ++i) {   // good: i is local to for-loop
        // ...
    }
    if (auto pc = dynamic_cast<Circle*>(ps)) {   // good: pc is local to if-statement
        // ... deal with Circle ...
    }
    else {
        // ... handle error ...
    }
}
Example, don't
int j;                            // BAD: j is visible outside the loop
for (j = 0; j < 100; ++j) {
    // ...
}
// j is still visible here and isn't needed

See also: (Don't use a variable for two unrelated purposes)

Enforcement
  • Warn when a variable modified inside the for-statement is declared outside the loop and not being used outside the loop.
  • (hard) Flag loop variables declared before the loop and used after the loop for an unrelated purpose.

Discussion: Scoping the loop variable to the loop body also helps code optimizers greatly. Recognizing that the induction variable is only accessible in the loop body unblocks optimizations such as hoisting, strength reduction, loop-invariant code motion, etc.

C++17 and C++20 example

Note: C++17 and C++20 also add if, switch, and range-for initializer statements. These require C++17 and C++20 support.

map<int, string> mymap;
if (auto result = mymap.insert(value); result.second) {
    // insert succeeded, and result is valid for this block
    use(result.first);  // ok
    // ...
} // result is destroyed here
C++17 and C++20 enforcement (if using a C++17 or C++20 compiler)
  • Flag selection/loop variables declared before the body and not used after the body
  • (hard) Flag selection/loop variables declared before the body and used after the body for an unrelated purpose.

ES.7: Keep common and local names short, and keep uncommon and non-local names longer

Reason

Readability. Lowering the chance of clashes between unrelated non-local names.

Example

Conventional short, local names increase readability:

template<typename T>    // good
void print(ostream& os, const vector<T>& v)
{
    for (gsl::index i = 0; i < v.size(); ++i)
        os << v[i] << '\n';
}

An index is conventionally called i and there is no hint about the meaning of the vector in this generic function, so v is as good name as any. Compare

template<typename Element_type>   // bad: verbose, hard to read
void print(ostream& target_stream, const vector<Element_type>& current_vector)
{
    for (gsl::index current_element_index = 0;
         current_element_index < current_vector.size();
         ++current_element_index
    )
    target_stream << current_vector[current_element_index] << '\n';
}

Yes, it is a caricature, but we have seen worse.

Example

Unconventional and short non-local names obscure code:

void use1(const string& s)
{
    // ...
    tt(s);   // bad: what is tt()?
    // ...
}

Better, give non-local entities readable names:

void use1(const string& s)
{
    // ...
    trim_tail(s);   // better
    // ...
}

Here, there is a chance that the reader knows what trim_tail means and that the reader can remember it after looking it up.

Example, bad

Argument names of large functions are de facto non-local and should be meaningful:

void complicated_algorithm(vector<Record>& vr, const vector<int>& vi, map<string, int>& out)
// read from events in vr (marking used Records) for the indices in
// vi placing (name, index) pairs into out
{
    // ... 500 lines of code using vr, vi, and out ...
}

We recommend keeping functions short, but that rule isn't universally adhered to and naming should reflect that.

Enforcement

Check length of local and non-local names. Also take function length into account.

ES.8: Avoid similar-looking names

Reason

Code clarity and readability. Too-similar names slow down comprehension and increase the likelihood of error.

Example, bad
if (readable(i1 + l1 + ol + o1 + o0 + ol + o1 + I0 + l0)) surprise();
Example, bad

Do not declare a non-type with the same name as a type in the same scope. This removes the need to disambiguate with a keyword such as struct or enum. It also removes a source of errors, as struct X can implicitly declare X if lookup fails.

struct foo { int n; };
struct foo foo();       // BAD, foo is a type already in scope
struct foo x = foo();   // requires disambiguation
Exception

Antique header files might declare non-types and types with the same name in the same scope.

Enforcement
  • Check names against a list of known confusing letter and digit combinations.
  • Flag a declaration of a variable, function, or enumerator that hides a class or enumeration declared in the same scope.

ES.9: Avoid ALL_CAPS names

Reason

Such names are commonly used for macros. Thus, ALL_CAPS name are vulnerable to unintended macro substitution.

Example
// somewhere in some header:
#define NE !=
// somewhere else in some other header:
enum Coord { N, NE, NW, S, SE, SW, E, W };
// somewhere third in some poor programmer's .cpp:
switch (direction) {
case N:
    // ...
case NE:
    // ...
// ...
}
Note

Do not use ALL_CAPS for constants just because constants used to be macros.

Enforcement

Flag all uses of ALL CAPS. For older code, accept ALL CAPS for macro names and flag all non-ALL-CAPS macro names.

ES.10: Declare one name (only) per declaration

Reason

One declaration per line increases readability and avoids mistakes related to the C/C++ grammar. It also leaves room for a more descriptive end-of-line comment.

Example, bad
char *p, c, a[7], *pp[7], **aa[10];   // yuck!
Exception

A function declaration can contain several function argument declarations.

Exception

A structured binding (C++17) is specifically designed to introduce several variables:

auto [iter, inserted] = m.insert_or_assign(k, val);
if (inserted) { /* new entry was inserted */ }
Example
template<class InputIterator, class Predicate>
bool any_of(InputIterator first, InputIterator last, Predicate pred);

or better using concepts:

bool any_of(input_iterator auto first, input_iterator auto last, predicate auto pred);
Example
double scalbn(double x, int n);   // OK: x * pow(FLT_RADIX, n); FLT_RADIX is usually 2

or:

double scalbn(    // better: x * pow(FLT_RADIX, n); FLT_RADIX is usually 2
    double x,     // base value
    int n         // exponent
);

or:

// better: base * pow(FLT_RADIX, exponent); FLT_RADIX is usually 2
double scalbn(double base, int exponent);
Example
int a = 10, b = 11, c = 12, d, e = 14, f = 15;

In a long list of declarators it is easy to overlook an uninitialized variable.

Enforcement

Flag variable and constant declarations with multiple declarators (e.g., int* p, q;)

ES.11: Use auto to avoid redundant repetition of type names

Reason
  • Simple repetition is tedious and error-prone.
  • When you use auto, the name of the declared entity is in a fixed position in the declaration, increasing readability.
  • In a function template declaration the return type can be a member type.
Example

Consider:

auto p = v.begin();      // vector<DataRecord>::iterator
auto z1 = v[3];          // makes copy of DataRecord
auto& z2 = v[3];         // avoids copy
const auto& z3 = v[3];   // const and avoids copy
auto h = t.future();
auto q = make_unique<int[]>(s);
auto f = [](int x) { return x + 10; };

In each case, we save writing a longish, hard-to-remember type that the compiler already knows but a programmer could get wrong.

Example
template<class T>
auto Container<T>::first() -> Iterator;   // Container<T>::Iterator
Exception

Avoid auto for initializer lists and in cases where you know exactly which type you want and where an initializer might require conversion.

Example
auto lst = { 1, 2, 3 };   // lst is an initializer list
auto x{1};   // x is an int (in C++17; initializer_list in C++11)
Note

As of C++20, we can (and should) use concepts to be more specific about the type we are deducing:

// ...
forward_iterator auto p = algo(x, y, z);
Example (C++17)
std::set<int> values;
// ...
auto [ position, newly_inserted ] = values.insert(5);   // break out the members of the std::pair
Enforcement

Flag redundant repetition of type names in a declaration.

ES.12: Do not reuse names in nested scopes

Reason

It is easy to get confused about which variable is used. Can cause maintenance problems.

Example, bad
int d = 0;
// ...
if (cond) {
    // ...
    d = 9;
    // ...
}
else {
    // ...
    int d = 7;
    // ...
    d = value_to_be_returned;
    // ...
}
return d;

If this is a large if-statement, it is easy to overlook that a new d has been introduced in the inner scope. This is a known source of bugs. Sometimes such reuse of a name in an inner scope is called "shadowing".

Note

Shadowing is primarily a problem when functions are too large and too complex.

Example

Shadowing of function arguments in the outermost block is disallowed by the language:

void f(int x)
{
    int x = 4;  // error: reuse of function argument name
    if (x) {
        int x = 7;  // allowed, but bad
        // ...
    }
}
Example, bad

Reuse of a member name as a local variable can also be a problem:

struct S {
    int m;
    void f(int x);
};
void S::f(int x)
{
    m = 7;    // assign to member
    if (x) {
        int m = 9;
        // ...
        m = 99; // assign to local variable
        // ...
    }
}
Exception

We often reuse function names from a base class in a derived class:

struct B {
    void f(int);
};
struct D : B {
    void f(double);
    using B::f;
};

This is error-prone. For example, had we forgotten the using declaration, a call d.f(1) would not have found the int version of f.

??? Do we need a specific rule about shadowing/hiding in class hierarchies?

Enforcement
  • Flag reuse of a name in nested local scopes
  • Flag reuse of a member name as a local variable in a member function
  • Flag reuse of a global name as a local variable or a member name
  • Flag reuse of a base class member name in a derived class (except for function names)

ES.20: Always initialize an object

Reason

Avoid used-before-set errors and their associated undefined behavior. Avoid problems with comprehension of complex initialization. Simplify refactoring.

Example
void use(int arg)
{
    int i;   // bad: uninitialized variable
    // ...
    i = 7;   // initialize i
}

No, i = 7 does not initialize i; it assigns to it. Also, i can be read in the ... part. Better:

void use(int arg)   // OK
{
    int i = 7;   // OK: initialized
    string s;    // OK: default initialized
    // ...
}
Note

The always initialize rule is deliberately stronger than the an object must be set before used language rule. The latter, more relaxed rule, catches the technical bugs, but:

  • It leads to less readable code
  • It encourages people to declare names in greater than necessary scopes
  • It leads to harder to read code
  • It leads to logic bugs by encouraging complex code
  • It hampers refactoring

The always initialize rule is a style rule aimed to improve maintainability as well as a rule protecting against used-before-set errors.

Example

Here is an example that is often considered to demonstrate the need for a more relaxed rule for initialization

widget i;    // "widget" a type that's expensive to initialize, possibly a large POD
widget j;
if (cond) {  // bad: i and j are initialized "late"
    i = f1();
    j = f2();
}
else {
    i = f3();
    j = f4();
}

This cannot trivially be rewritten to initialize i and j with initializers. Note that for types with a default constructor, attempting to postpone initialization simply leads to a default initialization followed by an assignment. A popular reason for such examples is "efficiency", but a compiler that can detect whether we made a used-before-set error can also eliminate any redundant double initialization.

Assuming that there is a logical connection between i and j, that connection should probably be expressed in code:

pair<widget, widget> make_related_widgets(bool x)
{
    return (x) ? {f1(), f2()} : {f3(), f4()};
}
auto [i, j] = make_related_widgets(cond);    // C++17

If the make_related_widgets function is otherwise redundant, we can eliminate it by using a lambda (ES.28):

auto [i, j] = [x] { return (x) ? pair{f1(), f2()} : pair{f3(), f4()} }();    // C++17

Using a value representing "uninitialized" is a symptom of a problem and not a solution:

widget i = uninit;  // bad
widget j = uninit;
// ...
use(i);         // possibly used before set
// ...
if (cond) {     // bad: i and j are initialized "late"
    i = f1();
    j = f2();
}
else {
    i = f3();
    j = f4();
}

Now the compiler cannot even simply detect a used-before-set. Further, we've introduced complexity in the state space for widget: which operations are valid on an uninit widget and which are not?

Note

Complex initialization has been popular with clever programmers for decades. It has also been a major source of errors and complexity. Many such errors are introduced during maintenance years after the initial implementation.

Example

This rule covers member variables.

class X {
public:
    X(int i, int ci) : m2{i}, cm2{ci} {}
    // ...
private:
    int m1 = 7;
    int m2;
    int m3;
    const int cm1 = 7;
    const int cm2;
    const int cm3;
};

The compiler will flag the uninitialized cm3 because it is a const, but it will not catch the lack of initialization of m3. Usually, a rare spurious member initialization is worth the absence of errors from lack of initialization and often an optimizer can eliminate a redundant initialization (e.g., an initialization that occurs immediately before an assignment).

Exception

If you are declaring an object that is just about to be initialized from input, initializing it would cause a double initialization. However, beware that this might leave uninitialized data beyond the input -- and that has been a fertile source of errors and security breaches:

constexpr int max = 8 * 1024;
int buf[max];         // OK, but suspicious: uninitialized
f.read(buf, max);

The cost of initializing that array could be significant in some situations. However, such examples do tend to leave uninitialized variables accessible, so they should be treated with suspicion.

constexpr int max = 8 * 1024;
int buf[max] = {};   // zero all elements; better in some situations
f.read(buf, max);

Because of the restrictive initialization rules for arrays and std::array, they offer the most compelling examples of the need for this exception.

When feasible use a library function that is known not to overflow. For example:

string s;   // s is default initialized to ""
cin >> s;   // s expands to hold the string

Don't consider simple variables that are targets for input operations exceptions to this rule:

int i;   // bad
// ...
cin >> i;

In the not uncommon case where the input target and the input operation get separated (as they should not) the possibility of used-before-set opens up.

int i2 = 0;   // better, assuming that zero is an acceptable value for i2
// ...
cin >> i2;

A good optimizer should know about input operations and eliminate the redundant operation.

Note

Sometimes, a lambda can be used as an initializer to avoid an uninitialized variable:

error_code ec;
Value v = [&] {
    auto p = get_value();   // get_value() returns a pair<error_code, Value>
    ec = p.first;
    return p.second;
}();

or maybe:

Value v = [] {
    auto p = get_value();   // get_value() returns a pair<error_code, Value>
    if (p.first) throw Bad_value{p.first};
    return p.second;
}();

See also: (ES.28)

Enforcement
  • Flag every uninitialized variable. Don't flag variables of user-defined types with default constructors.
  • Check that an uninitialized buffer is written into immediately after declaration. Passing an uninitialized variable as a reference to non-const argument can be assumed to be a write into the variable.

ES.21: Don't introduce a variable (or constant) before you need to use it

Reason

Readability. To limit the scope in which the variable can be used.

Example
int x = 7;
// ... no use of x here ...
++x;
Enforcement

Flag declarations that are distant from their first use.

ES.22: Don't declare a variable until you have a value to initialize it with

Reason

Readability. Limit the scope in which a variable can be used. Don't risk used-before-set. Initialization is often more efficient than assignment.

Example, bad
string s;
// ... no use of s here ...
s = "what a waste";
Example, bad
SomeLargeType var;  // Hard-to-read CaMeLcAsEvArIaBlE
if (cond)   // some non-trivial condition
    Set(&var);
else if (cond2 || !cond3) {
    var = Set2(3.14);
}
else {
    var = 0;
    for (auto& e : something)
        var += e;
}
// use var; that this isn't done too early can be enforced statically with only control flow

This would be fine if there was a default initialization for SomeLargeType that wasn't too expensive. Otherwise, a programmer might very well wonder if every possible path through the maze of conditions has been covered. If not, we have a "use before set" bug. This is a maintenance trap.

For initializers of moderate complexity, including for const variables, consider using a lambda to express the initializer; see (ES.28).

Enforcement
  • Flag declarations with default initialization that are assigned to before they are first read.
  • Flag any complicated computation after an uninitialized variable and before its use.

ES.23: Prefer the {}-initializer syntax

Reason

Prefer {}. The rules for {} initialization are simpler, more general, less ambiguous, and safer than for other forms of initialization.

Use = only when you are sure that there can be no narrowing conversions. For built-in arithmetic types, use = only with auto.

Avoid () initialization, which allows parsing ambiguities.

Example
int x {f(99)};
int y = x;
vector<int> v = {1, 2, 3, 4, 5, 6};
Exception

For containers, there is a tradition for using {...} for a list of elements and (...) for sizes:

vector<int> v1(10);    // vector of 10 elements with the default value 0
vector<int> v2{10};    // vector of 1 element with the value 10
vector<int> v3(1, 2);  // vector of 1 element with the value 2
vector<int> v4{1, 2};  // vector of 2 element with the values 1 and 2
Note

{}-initializers do not allow narrowing conversions (and that is usually a good thing) and allow explicit constructors (which is fine, we're intentionally initializing a new variable).

Example
int x {7.9};   // error: narrowing
int y = 7.9;   // OK: y becomes 7. Hope for a compiler warning
int z = gsl::narrow_cast<int>(7.9);  // OK: you asked for it
Note

{} initialization can be used for nearly all initialization; other forms of initialization can't:

auto p = new vector<int> {1, 2, 3, 4, 5};   // initialized vector
D::D(int a, int b) :m{a, b} {   // member initializer (e.g., m might be a pair)
    // ...
};
X var {};   // initialize var to be empty
struct S {
    int m {7};   // default initializer for a member
    // ...
};

For that reason, {}-initialization is often called "uniform initialization" (though there unfortunately are a few irregularities left).

Note

Initialization of a variable declared using auto with a single value, e.g., {v}, had surprising results until C++17. The C++17 rules are somewhat less surprising:

auto x1 {7};        // x1 is an int with the value 7
auto x2 = {7};      // x2 is an initializer_list<int> with an element 7
auto x11 {7, 8};    // error: two initializers
auto x22 = {7, 8};  // x22 is an initializer_list<int> with elements 7 and 8

Use ={...} if you really want an initializer_list<T>

auto fib10 = {1, 1, 2, 3, 5, 8, 13, 21, 34, 55};   // fib10 is a list
Note

={} gives copy initialization whereas {} gives direct initialization. Like the distinction between copy-initialization and direct-initialization itself, this can lead to surprises. {} accepts explicit constructors; ={} does not. For example:

struct Z { explicit Z() {} };
Z z1{};     // OK: direct initialization, so we use explicit constructor
Z z2 = {};  // error: copy initialization, so we cannot use the explicit constructor

Use plain {}-initialization unless you specifically want to disable explicit constructors.

Example
template<typename T>
void f()
{
    T x1(1);    // T initialized with 1
    T x0();     // bad: function declaration (often a mistake)
    T y1 {1};   // T initialized with 1
    T y0 {};    // default initialized T
    // ...
}

See also: Discussion

Enforcement
  • Flag uses of = to initialize arithmetic types where narrowing occurs.
  • Flag uses of () initialization syntax that are actually declarations. (Many compilers should warn on this already.)

ES.24: Use a unique_ptr<T> to hold pointers

Reason

Using std::unique_ptr is the simplest way to avoid leaks. It is reliable, it makes the type system do much of the work to validate ownership safety, it increases readability, and it has zero or near zero run-time cost.

Example
void use(bool leak)
{
    auto p1 = make_unique<int>(7);   // OK
    int* p2 = new int{7};            // bad: might leak
    // ... no assignment to p2 ...
    if (leak) return;
    // ... no assignment to p2 ...
    vector<int> v(7);
    v.at(7) = 0;                    // exception thrown
    // ...
}

If leak == true the object pointed to by p2 is leaked and the object pointed to by p1 is not. The same is the case when at() throws.

Enforcement

Look for raw pointers that are targets of new, malloc(), or functions that might return such pointers.

ES.25: Declare an object const or constexpr unless you want to modify its value later on

Reason

That way you can't change the value by mistake. That way might offer the compiler optimization opportunities.

Example
void f(int n)
{
    const int bufmax = 2 * n + 2;  // good: we can't change bufmax by accident
    int xmax = n;                  // suspicious: is xmax intended to change?
    // ...
}
Enforcement

Look to see if a variable is actually mutated, and flag it if not. Unfortunately, it might be impossible to detect when a non-const was not intended to vary (vs when it merely did not vary).

ES.26: Don't use a variable for two unrelated purposes

Reason

Readability and safety.

Example, bad
void use()
{
    int i;
    for (i = 0; i < 20; ++i) { /* ... */ }
    for (i = 0; i < 200; ++i) { /* ... */ } // bad: i recycled
}
Note

As an optimization, you might want to reuse a buffer as a scratch pad, but even then prefer to limit the variable's scope as much as possible and be careful not to cause bugs from data left in a recycled buffer as this is a common source of security bugs.

void write_to_file()
{
    std::string buffer;             // to avoid reallocations on every loop iteration
    for (auto& o : objects) {
        // First part of the work.
        generate_first_string(buffer, o);
        write_to_file(buffer);
        // Second part of the work.
        generate_second_string(buffer, o);
        write_to_file(buffer);
        // etc...
    }
}
Enforcement

Flag recycled variables.

ES.27: Use std::array or stack_array for arrays on the stack

Reason

They are readable and don't implicitly convert to pointers. They are not confused with non-standard extensions of built-in arrays.

Example, bad
const int n = 7;
int m = 9;
void f()
{
    int a1[n];
    int a2[m];   // error: not ISO C++
    // ...
}
Note

The definition of a1 is legal C++ and has always been. There is a lot of such code. It is error-prone, though, especially when the bound is non-local. Also, it is a "popular" source of errors (buffer overflow, pointers from array decay, etc.). The definition of a2 is C but not C++ and is considered a security risk

Example
const int n = 7;
int m = 9;
void f()
{
    array<int, n> a1;
    stack_array<int> a2(m);
    // ...
}
Enforcement
  • Flag arrays with non-constant bounds (C-style VLAs)
  • Flag arrays with non-local constant bounds

ES.28: Use lambdas for complex initialization, especially of const variables

Reason

It nicely encapsulates local initialization, including cleaning up scratch variables needed only for the initialization, without needing to create a needless non-local yet non-reusable function. It also works for variables that should be const but only after some initialization work.

Example, bad
widget x;   // should be const, but:
for (auto i = 2; i <= N; ++i) {          // this could be some
    x += some_obj.do_something_with(i);  // arbitrarily long code
}                                        // needed to initialize x
// from here, x should be const, but we can't say so in code in this style
Example, good
const widget x = [&] {
    widget val;                                // assume that widget has a default constructor
    for (auto i = 2; i <= N; ++i) {            // this could be some
        val += some_obj.do_something_with(i);  // arbitrarily long code
    }                                          // needed to initialize x
    return val;
}();

If at all possible, reduce the conditions to a simple set of alternatives (e.g., an enum) and don't mix up selection and initialization.

Enforcement

Hard. At best a heuristic. Look for an uninitialized variable followed by a loop assigning to it.

ES.30: Don't use macros for program text manipulation

Reason

Macros are a major source of bugs. Macros don't obey the usual scope and type rules. Macros ensure that the human reader sees something different from what the compiler sees. Macros complicate tool building.

Example, bad
#define Case break; case   /* BAD */

This innocuous-looking macro makes a single lower case c instead of a C into a bad flow-control bug.

Note

This rule does not ban the use of macros for "configuration control" use in #ifdefs, etc.

In the future, modules are likely to eliminate the need for macros in configuration control.

Note

This rule is meant to also discourage use of # for stringification and ## for concatenation. As usual for macros, there are uses that are "mostly harmless", but even these can create problems for tools, such as auto completers, static analyzers, and debuggers. Often the desire to use fancy macros is a sign of an overly complex design. Also, # and ## encourages the definition and use of macros:

#define CAT(a, b) a ## b
#define STRINGIFY(a) #a
void f(int x, int y)
{
    string CAT(x, y) = "asdf";   // BAD: hard for tools to handle (and ugly)
    string sx2 = STRINGIFY(x);
    // ...
}

There are workarounds for low-level string manipulation using macros. For example:

string s = "asdf" "lkjh";   // ordinary string literal concatenation
enum E { a, b };
template<int x>
constexpr const char* stringify()
{
    switch (x) {
    case a: return "a";
    case b: return "b";
    }
}
void f(int x, int y)
{
    string sx = stringify<x>();
    // ...
}

This is not as convenient as a macro to define, but as easy to use, has zero overhead, and is typed and scoped.

In the future, static reflection is likely to eliminate the last needs for the preprocessor for program text manipulation.

Enforcement

Scream when you see a macro that isn't just used for source control (e.g., #ifdef)

ES.31: Don't use macros for constants or "functions"

Reason

Macros are a major source of bugs. Macros don't obey the usual scope and type rules. Macros don't obey the usual rules for argument passing. Macros ensure that the human reader sees something different from what the compiler sees. Macros complicate tool building.

Example, bad
#define PI 3.14
#define SQUARE(a, b) (a * b)

Even if we hadn't left a well-known bug in SQUARE there are much better behaved alternatives; for example:

constexpr double pi = 3.14;
template<typename T> T square(T a, T b) { return a * b; }
Enforcement

Scream when you see a macro that isn't just used for source control (e.g., #ifdef)

ES.32: Use ALL_CAPS for all macro names

Reason

Convention. Readability. Distinguishing macros.

Example
#define forever for (;;)   /* very BAD */
#define FOREVER for (;;)   /* Still evil, but at least visible to humans */
Enforcement

Scream when you see a lower case macro.

ES.33: If you must use macros, give them unique names

Reason

Macros do not obey scope rules.

Example
#define MYCHAR        /* BAD, will eventually clash with someone else's MYCHAR*/
#define ZCORP_CHAR    /* Still evil, but less likely to clash */
Note

Avoid macros if you can: ((ES.30), ES.31), and ES.32. However, there are billions of lines of code littered with macros and a long tradition for using and overusing macros. If you are forced to use macros, use long names and supposedly unique prefixes (e.g., your organization's name) to lower the likelihood of a clash.

Enforcement

Warn against short macro names.

ES.34: Don't define a (C-style) variadic function

Reason

Not type safe. Requires messy cast-and-macro-laden code to get working right.

Example
#include <cstdarg>
// "severity" followed by a zero-terminated list of char*s; write the C-style strings to cerr
void error(int severity ...)
{
    va_list ap;             // a magic type for holding arguments
    va_start(ap, severity); // arg startup: "severity" is the first argument of error()
    for (;;) {
        // treat the next var as a char*; no checking: a cast in disguise
        char* p = va_arg(ap, char*);
        if (!p) break;
        cerr << p << ' ';
    }
    va_end(ap);             // arg cleanup (don't forget this)
    cerr << '\n';
    if (severity) exit(severity);
}
void use()
{
    error(7, "this", "is", "an", "error", nullptr);
    error(7); // crash
    error(7, "this", "is", "an", "error");  // crash
    const char* is = "is";
    string an = "an";
    error(7, "this", "is", an, "error"); // crash
}

Alternative: Overloading. Templates. Variadic templates.

#include <iostream>
void error(int severity)
{
    std::cerr << '\n';
    std::exit(severity);
}
template<typename T, typename... Ts>
constexpr void error(int severity, T head, Ts... tail)
{
    std::cerr << head;
    error(severity, tail...);
}
void use()
{
    error(7); // No crash!
    error(5, "this", "is", "not", "an", "error"); // No crash!
    std::string an = "an";
    error(7, "this", "is", "not", an, "error"); // No crash!
    error(5, "oh", "no", nullptr); // Compile error! No need for nullptr.
}
Note

This is basically the way printf is implemented.

Enforcement
  • Flag definitions of C-style variadic functions.
  • Flag #include <cstdarg> and #include <stdarg.h>

ES.expr: Expressions

Expressions manipulate values.

ES.40: Avoid complicated expressions

Reason

Complicated expressions are error-prone.

Example
// bad: assignment hidden in subexpression
while ((c = getc()) != -1)
// bad: two non-local variables assigned in sub-expressions
while ((cin >> c1, cin >> c2), c1 == c2)
// better, but possibly still too complicated
for (char c1, c2; cin >> c1 >> c2 && c1 == c2;)
// OK: if i and j are not aliased
int x = ++i + ++j;
// OK: if i != j and i != k
v[i] = v[j] + v[k];
// bad: multiple assignments "hidden" in subexpressions
x = a + (b = f()) + (c = g()) * 7;
// bad: relies on commonly misunderstood precedence rules
x = a & b + c * d && e ^ f == 7;
// bad: undefined behavior
x = x++ + x++ + ++x;

Some of these expressions are unconditionally bad (e.g., they rely on undefined behavior). Others are simply so complicated and/or unusual that even good programmers could misunderstand them or overlook a problem when in a hurry.

Note

C++17 tightens up the rules for the order of evaluation (left-to-right except right-to-left in assignments, and the order of evaluation of function arguments is unspecified; (see ES.43)), but that doesn't change the fact that complicated expressions are potentially confusing.

Note

A programmer should know and use the basic rules for expressions.

Example
x = k * y + z;             // OK
auto t1 = k * y;           // bad: unnecessarily verbose
x = t1 + z;
if (0 <= x && x < max)   // OK
auto t1 = 0 <= x;        // bad: unnecessarily verbose
auto t2 = x < max;
if (t1 && t2)            // ...
Enforcement

Tricky. How complicated must an expression be to be considered complicated? Writing computations as statements with one operation each is also confusing. Things to consider:

  • side effects: side effects on multiple non-local variables (for some definition of non-local) can be suspect, especially if the side effects are in separate subexpressions
  • writes to aliased variables
  • more than N operators (and what should N be?)
  • reliance of subtle precedence rules
  • uses undefined behavior (can we catch all undefined behavior?)
  • implementation defined behavior?
  • ???

ES.41: If in doubt about operator precedence, parenthesize

Reason

Avoid errors. Readability. Not everyone has the operator table memorized.

Example
const unsigned int flag = 2;
unsigned int a = flag;
if (a & flag != 0)  // bad: means a&(flag != 0)

Note: We recommend that programmers know their precedence table for the arithmetic operations, the logical operations, but consider mixing bitwise logical operations with other operators in need of parentheses.

if ((a & flag) != 0)  // OK: works as intended
Note

You should know enough not to need parentheses for:

if (a < 0 || a <= max) {
    // ...
}
Enforcement
  • Flag combinations of bitwise-logical operators and other operators.
  • Flag assignment operators not as the leftmost operator.
  • ???

ES.42: Keep use of pointers simple and straightforward

Reason

Complicated pointer manipulation is a major source of errors.

Note

Use gsl::span instead. Pointers should (only refer to single objects). Pointer arithmetic is fragile and easy to get wrong, the source of many, many bad bugs and security violations. span is a bounds-checked, safe type for accessing arrays of data. Access into an array with known bounds using a constant as a subscript can be validated by the compiler.

Example, bad
void f(int* p, int count)
{
    if (count < 2) return;
    int* q = p + 1;    // BAD
    ptrdiff_t d;
    int n;
    d = (p - &n);      // OK
    d = (q - p);       // OK
    int n = *p++;      // BAD
    if (count < 6) return;
    p[4] = 1;          // BAD
    p[count - 1] = 2;  // BAD
    use(&p[0], 3);     // BAD
}
Example, good
void f(span<int> a) // BETTER: use span in the function declaration
{
    if (a.size() < 2) return;
    int n = a[0];      // OK
    span<int> q = a.subspan(1); // OK
    if (a.size() < 6) return;
    a[4] = 1;          // OK
    a[a.size() - 1] = 2;  // OK
    use(a.data(), 3);  // OK
}
Note

Subscripting with a variable is difficult for both tools and humans to validate as safe. span is a run-time bounds-checked, safe type for accessing arrays of data. at() is another alternative that ensures single accesses are bounds-checked. If iterators are needed to access an array, use the iterators from a span constructed over the array.

Example, bad
void f(array<int, 10> a, int pos)
{
    a[pos / 2] = 1; // BAD
    a[pos - 1] = 2; // BAD
    a[-1] = 3;    // BAD (but easily caught by tools) -- no replacement, just don't do this
    a[10] = 4;    // BAD (but easily caught by tools) -- no replacement, just don't do this
}
Example, good

Use a span:

void f1(span<int, 10> a, int pos) // A1: Change parameter type to use span
{
    a[pos / 2] = 1; // OK
    a[pos - 1] = 2; // OK
}
void f2(array<int, 10> arr, int pos) // A2: Add local span and use that
{
    span<int> a = {arr.data(), pos};
    a[pos / 2] = 1; // OK
    a[pos - 1] = 2; // OK
}

Use at():

void f3(array<int, 10> a, int pos) // ALTERNATIVE B: Use at() for access
{
    at(a, pos / 2) = 1; // OK
    at(a, pos - 1) = 2; // OK
}
Example, bad
void f()
{
    int arr[COUNT];
    for (int i = 0; i < COUNT; ++i)
        arr[i] = i; // BAD, cannot use non-constant indexer
}
Example, good

Use a span:

void f1()
{
    int arr[COUNT];
    span<int> av = arr;
    for (int i = 0; i < COUNT; ++i)
        av[i] = i;
}

Use a span and range-for:

void f1a()
{
     int arr[COUNT];
     span<int, COUNT> av = arr;
     int i = 0;
     for (auto& e : av)
         e = i++;
}

Use at() for access:

void f2()
{
    int arr[COUNT];
    for (int i = 0; i < COUNT; ++i)
        at(arr, i) = i;
}

Use a range-for:

void f3()
{
    int arr[COUNT];
    int i = 0;
    for (auto& e : arr)
         e = i++;
}
Note

Tooling can offer rewrites of array accesses that involve dynamic index expressions to use at() instead:

static int a[10];
void f(int i, int j)
{
    a[i + j] = 12;      // BAD, could be rewritten as ...
    at(a, i + j) = 12;  // OK -- bounds-checked
}
Example

Turning an array into a pointer (as the language does essentially always) removes opportunities for checking, so avoid it

void g(int* p);
void f()
{
    int a[5];
    g(a);        // BAD: are we trying to pass an array?
    g(&a[0]);    // OK: passing one object
}

If you want to pass an array, say so:

void g(int* p, size_t length);  // old (dangerous) code
void g1(span<int> av); // BETTER: get g() changed.
void f2()
{
    int a[5];
    span<int> av = a;
    g(av.data(), av.size());   // OK, if you have no choice
    g1(a);                     // OK -- no decay here, instead use implicit span ctor
}
Enforcement
  • Flag any arithmetic operation on an expression of pointer type that results in a value of pointer type.
  • Flag any indexing expression on an expression or variable of array type (either static array or std::array) where the indexer is not a compile-time constant expression with a value between 0 and the upper bound of the array.
  • Flag any expression that would rely on implicit conversion of an array type to a pointer type.

This rule is part of the (bounds-safety profile).

ES.43: Avoid expressions with undefined order of evaluation

Reason

You have no idea what such code does. Portability. Even if it does something sensible for you, it might do something different on another compiler (e.g., the next release of your compiler) or with a different optimizer setting.

Note

C++17 tightens up the rules for the order of evaluation: left-to-right except right-to-left in assignments, and the order of evaluation of function arguments is unspecified.

However, remember that your code might be compiled with a pre-C++17 compiler (e.g., through cut-and-paste) so don't be too clever.

Example
v[i] = ++i;   //  the result is undefined

A good rule of thumb is that you should not read a value twice in an expression where you write to it.

Enforcement

Can be detected by a good analyzer.

ES.44: Don't depend on order of evaluation of function arguments

Reason

Because that order is unspecified.

Note

C++17 tightens up the rules for the order of evaluation, but the order of evaluation of function arguments is still unspecified.

Example
int i = 0;
f(++i, ++i);

Before C++17, the behavior is undefined, so the behavior could be anything (e.g., f(2, 2)). Since C++17, this code does not have undefined behavior, but it is still not specified which argument is evaluated first. The call will be f(1, 2) or f(2, 1), but you don't know which.

Example

Overloaded operators can lead to order of evaluation problems:

f1()->m(f2());          // m(f1(), f2())
cout << f1() << f2();   // operator<<(operator<<(cout, f1()), f2())

In C++17, these examples work as expected (left to right) and assignments are evaluated right to left (just as ='s binding is right-to-left)

f1() = f2();    // undefined behavior in C++14; in C++17, f2() is evaluated before f1()
Enforcement

Can be detected by a good analyzer.

ES.45: Avoid "magic constants"; use symbolic constants

Reason

Unnamed constants embedded in expressions are easily overlooked and often hard to understand:

Example
for (int m = 1; m <= 12; ++m)   // don't: magic constant 12
    cout << month[m] << '\n';

No, we don't all know that there are 12 months, numbered 1..12, in a year. Better:

// months are indexed 1..12
constexpr int first_month = 1;
constexpr int last_month = 12;
for (int m = first_month; m <= last_month; ++m)   // better
    cout << month[m] << '\n';

Better still, don't expose constants:

for (auto m : month)
    cout << m << '\n';
Enforcement

Flag literals in code. Give a pass to 0, 1, nullptr, \n, "", and others on a positive list.

ES.46: Avoid lossy (narrowing, truncating) arithmetic conversions

Reason

A narrowing conversion destroys information, often unexpectedly so.

Example, bad

A key example is basic narrowing:

double d = 7.9;
int i = d;    // bad: narrowing: i becomes 7
i = (int) d;  // bad: we're going to claim this is still not explicit enough
void f(int x, long y, double d)
{
    char c1 = x;   // bad: narrowing
    char c2 = y;   // bad: narrowing
    char c3 = d;   // bad: narrowing
}
Note

The guidelines support library offers a narrow_cast operation for specifying that narrowing is acceptable and a narrow ("narrow if") that throws an exception if a narrowing would throw away legal values:

i = gsl::narrow_cast<int>(d);   // OK (you asked for it): narrowing: i becomes 7
i = gsl::narrow<int>(d);        // OK: throws narrowing_error

We also include lossy arithmetic casts, such as from a negative floating point type to an unsigned integral type:

double d = -7.9;
unsigned u = 0;
u = d;                               // bad: narrowing
u = gsl::narrow_cast<unsigned>(d);   // OK (you asked for it): u becomes 4294967289
u = gsl::narrow<unsigned>(d);        // OK: throws narrowing_error
Note

This rule does not apply to contextual conversions to bool:

if (ptr) do_something(*ptr);   // OK: ptr is used as a condition
bool b = ptr;                  // bad: narrowing
Enforcement

A good analyzer can detect all narrowing conversions. However, flagging all narrowing conversions will lead to a lot of false positives. Suggestions:

  • Flag all floating-point to integer conversions (maybe only float->char and double->int. Here be dragons! we need data).
  • Flag all long->char (I suspect int->char is very common. Here be dragons! we need data).
  • Consider narrowing conversions for function arguments especially suspect.

ES.47: Use nullptr rather than 0 or NULL

Reason

Readability. Minimize surprises: nullptr cannot be confused with an int. nullptr also has a well-specified (very restrictive) type, and thus works in more scenarios where type deduction might do the wrong thing on NULL or 0.

Example

Consider:

void f(int);
void f(char*);
f(0);         // call f(int)
f(nullptr);   // call f(char*)
Enforcement

Flag uses of 0 and NULL for pointers. The transformation might be helped by simple program transformation.

ES.48: Avoid casts

Reason

Casts are a well-known source of errors and make some optimizations unreliable.

Example, bad
double d = 2;
auto p = (long*)&d;
auto q = (long long*)&d;
cout << d << ' ' << *p << ' ' << *q << '\n';

What would you think this fragment prints? The result is at best implementation defined. I got

2 0 4611686018427387904

Adding

*q = 666;
cout << d << ' ' << *p << ' ' << *q << '\n';

I got

3.29048e-321 666 666

Surprised? I'm just glad I didn't crash the program.

Note

Programmers who write casts typically assume that they know what they are doing, or that writing a cast makes the program "easier to read". In fact, they often disable the general rules for using values. Overload resolution and template instantiation usually pick the right function if there is a right function to pick. If there is not, maybe there ought to be, rather than applying a local fix (cast).

Notes

Casts are necessary in a systems programming language. For example, how else would we get the address of a device register into a pointer? However, casts are seriously overused as well as a major source of errors.

If you feel the need for a lot of casts, there might be a fundamental design problem.

The (type profile) bans reinterpret_cast and C-style casts.

Never cast to (void) to ignore a [[nodiscard]]return value. If you deliberately want to discard such a result, first think hard about whether that is really a good idea (there is usually a good reason the author of the function or of the return type used [[nodiscard]] in the first place). If you still think it's appropriate and your code reviewer agrees, use std::ignore = to turn off the warning which is simple, portable, and easy to grep.

Alternatives

Casts are widely (mis)used. Modern C++ has rules and constructs that eliminate the need for casts in many contexts, such as

  • Use templates
  • Use std::variant
  • Rely on the well-defined, safe, implicit conversions between pointer types
  • Use std::ignore = to ignore [[nodiscard]] values.
Enforcement
  • Flag all C-style casts, including to void.
  • Flag functional style casts using Type(value). Use Type{value} instead which is not narrowing. (See (ES.64).)
  • Flag (identity casts) between pointer types, where the source and target types are the same (#Pro-type-identitycast).
  • Flag an explicit pointer cast that could be (implicit).

ES.49: If you must use a cast, use a named cast

Reason

Readability. Error avoidance. Named casts are more specific than a C-style or functional cast, allowing the compiler to catch some errors.

The named casts are:

  • static_cast
  • const_cast
  • reinterpret_cast
  • dynamic_cast
  • std::move // move(x) is an rvalue reference to x
  • std::forward // forward<T>(x) is an rvalue or an lvalue reference to x depending on T
  • gsl::narrow_cast // narrow_cast<T>(x) is static_cast<T>(x)
  • gsl::narrow // narrow<T>(x) is static_cast<T>(x) if static_cast<T>(x) == x or it throws narrowing_error
Example
class B { /* ... */ };
class D { /* ... */ };
template<typename D> D* upcast(B* pb)
{
    D* pd0 = pb;                        // error: no implicit conversion from B* to D*
    D* pd1 = (D*)pb;                    // legal, but what is done?
    D* pd2 = static_cast<D*>(pb);       // error: D is not derived from B
    D* pd3 = reinterpret_cast<D*>(pb);  // OK: on your head be it!
    D* pd4 = dynamic_cast<D*>(pb);      // OK: return nullptr
    // ...
}

The example was synthesized from real-world bugs where D used to be derived from B, but someone refactored the hierarchy. The C-style cast is dangerous because it can do any kind of conversion, depriving us of any protection from mistakes (now or in the future).

Note

When converting between types with no information loss (e.g. from float to double or from int32 to int64), brace initialization might be used instead.

double d {some_float};
int64_t i {some_int32};

This makes it clear that the type conversion was intended and also prevents conversions between types that might result in loss of precision. (It is a compilation error to try to initialize a float from a double in this fashion, for example.)

Note

reinterpret_cast can be essential, but the essential uses (e.g., turning a machine address into pointer) are not type safe:

auto p = reinterpret_cast<Device_register>(0x800);  // inherently dangerous
Enforcement
  • Flag all C-style casts, including to void.
  • Flag functional style casts using Type(value). Use Type{value} instead which is not narrowing. (See (ES.64).)
  • The (type profile) bans reinterpret_cast.
  • The (type profile) warns when using static_cast between arithmetic types.

ES.50: Don't cast away const

Reason

It makes a lie out of const. If the variable is actually declared const, modifying it results in undefined behavior.

Example, bad
void f(const int& x)
{
    const_cast<int&>(x) = 42;   // BAD
}
static int i = 0;
static const int j = 0;
f(i); // silent side effect
f(j); // undefined behavior
Example

Sometimes, you might be tempted to resort to const_cast to avoid code duplication, such as when two accessor functions that differ only in const-ness have similar implementations. For example:

class Bar;
class Foo {
public:
    // BAD, duplicates logic
    Bar& get_bar()
    {
        /* complex logic around getting a non-const reference to my_bar */
    }
    const Bar& get_bar() const
    {
        /* same complex logic around getting a const reference to my_bar */
    }
private:
    Bar my_bar;
};

Instead, prefer to share implementations. Normally, you can just have the non-const function call the const function. However, when there is complex logic this can lead to the following pattern that still resorts to a const_cast:

class Foo {
public:
    // not great, non-const calls const version but resorts to const_cast
    Bar& get_bar()
    {
        return const_cast<Bar&>(static_cast<const Foo&>(*this).get_bar());
    }
    const Bar& get_bar() const
    {
        /* the complex logic around getting a const reference to my_bar */
    }
private:
    Bar my_bar;
};

Although this pattern is safe when applied correctly, because the caller must have had a non-const object to begin with, it's not ideal because the safety is hard to enforce automatically as a checker rule.

Instead, prefer to put the common code in a common helper function -- and make it a template so that it deduces const. This doesn't use any const_cast at all:

class Foo {
public:                         // good
          Bar& get_bar()       { return get_bar_impl(*this); }
    const Bar& get_bar() const { return get_bar_impl(*this); }
private:
    Bar my_bar;
    template<class T>           // good, deduces whether T is const or non-const
    static auto& get_bar_impl(T& t)
        { /* the complex logic around getting a possibly-const reference to my_bar */ }
};

Note: Don't do large non-dependent work inside a template, which leads to code bloat. For example, a further improvement would be if all or part of get_bar_impl can be non-dependent and factored out into a common non-template function, for a potentially big reduction in code size.

Exception

You might need to cast away const when calling const-incorrect functions. Prefer to wrap such functions in inline const-correct wrappers to encapsulate the cast in one place.

Example

Sometimes, "cast away const" is to allow the updating of some transient information of an otherwise immutable object. Examples are caching, memoization, and precomputation. Such examples are often handled as well or better using mutable or an indirection than with a const_cast.

Consider keeping previously computed results around for a costly operation:

int compute(int x); // compute a value for x; assume this to be costly
class Cache {   // some type implementing a cache for an int->int operation
public:
    pair<bool, int> find(int x) const;   // is there a value for x?
    void set(int x, int v);             // make y the value for x
    // ...
private:
    // ...
};
class X {
public:
    int get_val(int x)
    {
        auto p = cache.find(x);
        if (p.first) return p.second;
        int val = compute(x);
        cache.set(x, val); // insert value for x
        return val;
    }
    // ...
private:
    Cache cache;
};

Here, get_val() is logically constant, so we would like to make it a const member. To do this we still need to mutate cache, so people sometimes resort to a const_cast:

class X {   // Suspicious solution based on casting
public:
    int get_val(int x) const
    {
        auto p = cache.find(x);
        if (p.first) return p.second;
        int val = compute(x);
        const_cast<Cache&>(cache).set(x, val);   // ugly
        return val;
    }
    // ...
private:
    Cache cache;
};

Fortunately, there is a better solution: State that cache is mutable even for a const object:

class X {   // better solution
public:
    int get_val(int x) const
    {
        auto p = cache.find(x);
        if (p.first) return p.second;
        int val = compute(x);
        cache.set(x, val);
        return val;
    }
    // ...
private:
    mutable Cache cache;
};

An alternative solution would be to store a pointer to the cache:

class X {   // OK, but slightly messier solution
public:
    int get_val(int x) const
    {
        auto p = cache->find(x);
        if (p.first) return p.second;
        int val = compute(x);
        cache->set(x, val);
        return val;
    }
    // ...
private:
    unique_ptr<Cache> cache;
};

That solution is the most flexible, but requires explicit construction and destruction of *cache (most likely in the constructor and destructor of X).

In any variant, we must guard against data races on the cache in multi-threaded code, possibly using a std::mutex.

Enforcement

ES.55: Avoid the need for range checking

Reason

Constructs that cannot overflow do not overflow (and usually run faster):

Example
for (auto& x : v)      // print all elements of v
    cout << x << '\n';
auto p = find(v, x);   // find x in v
Enforcement

Look for explicit range checks and heuristically suggest alternatives.

ES.56: Write std::move() only when you need to explicitly move an object to another scope

Reason

We move, rather than copy, to avoid duplication and for improved performance.

A move typically leaves behind an empty object ((C.64)), which can be surprising or even dangerous, so we try to avoid moving from lvalues (they might be accessed later).

Notes

Moving is done implicitly when the source is an rvalue (e.g., value in a return treatment or a function result), so don't pointlessly complicate code in those cases by writing move explicitly. Instead, write short functions that return values, and both the function's return and the caller's accepting of the return will be optimized naturally.

In general, following the guidelines in this document (including not making variables' scopes needlessly large, writing short functions that return values, returning local variables) help eliminate most need for explicit std::move.

Explicit move is needed to explicitly move an object to another scope, notably to pass it to a "sink" function and in the implementations of the move operations themselves (move constructor, move assignment operator) and swap operations.

Example, bad
void sink(X&& x);   // sink takes ownership of x
void user()
{
    X x;
    // error: cannot bind an lvalue to a rvalue reference
    sink(x);
    // OK: sink takes the contents of x, x must now be assumed to be empty
    sink(std::move(x));
    // ...
    // probably a mistake
    use(x);
}

Usually, a std::move() is used as an argument to a && parameter. And after you do that, assume the object has been moved from (see (C.64)) and don't read its state again until you first set it to a new value.

void f()
{
    string s1 = "supercalifragilisticexpialidocious";
    string s2 = s1;             // ok, takes a copy
    assert(s1 == "supercalifragilisticexpialidocious");  // ok
    // bad, if you want to keep using s1's value
    string s3 = move(s1);
    // bad, assert will likely fail, s1 likely changed
    assert(s1 == "supercalifragilisticexpialidocious");
}
Example
void sink(unique_ptr<widget> p);  // pass ownership of p to sink()
void f()
{
    auto w = make_unique<widget>();
    // ...
    sink(std::move(w));               // ok, give to sink()
    // ...
    sink(w);    // Error: unique_ptr is carefully designed so that you cannot copy it
}
Notes

std::move() is a cast to && in disguise; it doesn't itself move anything, but marks a named object as a candidate that can be moved from. The language already knows the common cases where objects can be moved from, especially when returning values from functions, so don't complicate code with redundant std::move()'s.

Never write std::move() just because you've heard "it's more efficient." In general, don't believe claims of "efficiency" without data (???). In general, don't complicate your code without reason (??). Never write std::move() on a const object, it is silently transformed into a copy (see Item 23 in (Meyers15))

Example, bad
vector<int> make_vector()
{
    vector<int> result;
    // ... load result with data
    return std::move(result);       // bad; just write "return result;"
}

Never write return move(local_variable);, because the language already knows the variable is a move candidate. Writing move in this code won't help, and can actually be detrimental because on some compilers it interferes with RVO (the return value optimization) by creating an additional reference alias to the local variable.

Example, bad
vector<int> v = std::move(make_vector());   // bad; the std::move is entirely redundant

Never write move on a returned value such as x = move(f()); where f returns by value. The language already knows that a returned value is a temporary object that can be moved from.

Example
void mover(X&& x)
{
    call_something(std::move(x));         // ok
    call_something(std::forward<X>(x));   // bad, don't std::forward an rvalue reference
    call_something(x);                    // suspicious, why not std::move?
}
template<class T>
void forwarder(T&& t)
{
    call_something(std::move(t));         // bad, don't std::move a forwarding reference
    call_something(std::forward<T>(t));   // ok
    call_something(t);                    // suspicious, why not std::forward?
}
Enforcement
  • Flag use of std::move(x) where x is an rvalue or the language will already treat it as an rvalue, including return std::move(local_variable); and std::move(f()) on a function that returns by value.
  • Flag functions taking an S&& parameter if there is no const S& overload to take care of lvalues.
  • Flag a std::moves argument passed to a parameter, except when the parameter type is an X&& rvalue reference or the type is move-only and the parameter is passed by value.
  • Flag when std::move is applied to a forwarding reference (T&& where T is a template parameter type). Use std::forward instead.
  • Flag when std::move is applied to other than an rvalue reference to non-const. (More general case of the previous rule to cover the non-forwarding cases.)
  • Flag when std::forward is applied to an rvalue reference (X&& where X is a non-template parameter type). Use std::move instead.
  • Flag when std::forward is applied to other than a forwarding reference. (More general case of the previous rule to cover the non-moving cases.)
  • Flag when an object is potentially moved from and the next operation is a const operation; there should first be an intervening non-const operation, ideally assignment, to first reset the object's value.

ES.60: Avoid new and delete outside resource management functions

Reason

Direct resource management in application code is error-prone and tedious.

Note

This is also known as the rule of "No naked new!"

Example, bad
void f(int n)
{
    auto p = new X[n];   // n default constructed Xs
    // ...
    delete[] p;
}

There can be code in the ... part that causes the delete never to happen.

See also: (R: Resource management)

Enforcement

Flag naked news and naked deletes.

ES.61: Delete arrays using delete[] and non-arrays using delete

Reason

That's what the language requires and mistakes can lead to resource release errors and/or memory corruption.

Example, bad
void f(int n)
{
    auto p = new X[n];   // n default constructed Xs
    // ...
    delete p;   // error: just delete the object p, rather than delete the array p[]
}
Note

This example not only violates the (no naked new rule) as in the previous example, it has many more problems.

Enforcement
  • If the new and the delete are in the same scope, mistakes can be flagged.
  • If the new and the delete are in a constructor/destructor pair, mistakes can be flagged.

ES.62: Don't compare pointers into different arrays

Reason

The result of doing so is undefined.

Example, bad
void f()
{
    int a1[7];
    int a2[9];
    if (&a1[5] < &a2[7]) {}       // bad: undefined
    if (0 < &a1[5] - &a2[7]) {}   // bad: undefined
}
Note

This example has many more problems.

Enforcement

???

ES.63: Don't slice

Reason

Slicing -- that is, copying only part of an object using assignment or initialization -- most often leads to errors because the object was meant to be considered as a whole. In the rare cases where the slicing was deliberate the code can be surprising.

Example
class Shape { /* ... */ };
class Circle : public Shape { /* ... */ Point c; int r; };
Circle c {{0, 0}, 42};
Shape s {c};    // copy construct only the Shape part of Circle
s = c;          // or copy assign only the Shape part of Circle
void assign(const Shape& src, Shape& dest)
{
    dest = src;
}
Circle c2 {{1, 1}, 43};
assign(c, c2);   // oops, not the whole state is transferred
assert(c == c2); // if we supply copying, we should also provide comparison,
                 // but this will likely return false

The result will be meaningless because the center and radius will not be copied from c into s. The first defense against this is to (define the base class Shape not to allow this).

Alternative

If you mean to slice, define an explicit operation to do so. This saves readers from confusion. For example:

class Smiley : public Circle {
    public:
    Circle copy_circle();
    // ...
};
Smiley sm { /* ... */ };
Circle c1 {sm};  // ideally prevented by the definition of Circle
Circle c2 {sm.copy_circle()};
Enforcement

Warn against slicing.

ES.64: Use the T{e}notation for construction

Reason

The T{e} construction syntax makes it explicit that construction is desired. The T{e} construction syntax doesn't allow narrowing. T{e} is the only safe and general expression for constructing a value of type T from an expression e. The casts notations T(e) and (T)e are neither safe nor general.

Example

For built-in types, the construction notation protects against narrowing and reinterpretation

void use(char ch, int i, double d, char* p, long long lng)
{
    int x1 = int{ch};     // OK, but redundant
    int x2 = int{d};      // error: double->int narrowing; use a cast if you need to
    int x3 = int{p};      // error: pointer to->int; use a reinterpret_cast if you really need to
    int x4 = int{lng};    // error: long long->int narrowing; use a cast if you need to
    int y1 = int(ch);     // OK, but redundant
    int y2 = int(d);      // bad: double->int narrowing; use a cast if you need to
    int y3 = int(p);      // bad: pointer to->int; use a reinterpret_cast if you really need to
    int y4 = int(lng);    // bad: long long->int narrowing; use a cast if you need to
    int z1 = (int)ch;     // OK, but redundant
    int z2 = (int)d;      // bad: double->int narrowing; use a cast if you need to
    int z3 = (int)p;      // bad: pointer to->int; use a reinterpret_cast if you really need to
    int z4 = (int)lng;    // bad: long long->int narrowing; use a cast if you need to
}

The integer to/from pointer conversions are implementation defined when using the T(e) or (T)e notations, and non-portable between platforms with different integer and pointer sizes.

Note

((Avoid casts) (explicit type conversion) and if you must prefer named casts).

Note

When unambiguous, the T can be left out of T{e}.

complex<double> f(complex<double>);
auto z = f({2*pi, 1});
Note

The construction notation is the most general (initializer notation).

Exception

std::vector and other containers were defined before we had {} as a notation for construction. Consider:

vector<string> vs {10};                           // ten empty strings
vector<int> vi1 {1, 2, 3, 4, 5, 6, 7, 8, 9, 10};  // ten elements 1..10
vector<int> vi2 {10};                             // one element with the value 10

How do we get a vector of 10 default initialized ints?

vector<int> v3(10); // ten elements with value 0

The use of () rather than {} for number of elements is conventional (going back to the early 1980s), hard to change, but still a design error: for a container where the element type can be confused with the number of elements, we have an ambiguity that must be resolved. The conventional resolution is to interpret {10} as a list of one element and use (10) to distinguish a size.

This mistake need not be repeated in new code. We can define a type to represent the number of elements:

struct Count { int n; };
template<typename T>
class Vector {
public:
    Vector(Count n);                     // n default-initialized elements
    Vector(initializer_list<T> init);    // init.size() elements
    // ...
};
Vector<int> v1{10};
Vector<int> v2{Count{10}};
Vector<Count> v3{Count{10}};    // yes, there is still a very minor problem

The main problem left is to find a suitable name for Count.

Enforcement

Flag the C-style (T)e and functional-style T(e) casts.

ES.65: Don't dereference an invalid pointer

Reason

Dereferencing an invalid pointer, such as nullptr, is undefined behavior, typically leading to immediate crashes, wrong results, or memory corruption.

Note

This rule is an obvious and well-known language rule, but can be hard to follow. It takes good coding style, library support, and static analysis to eliminate violations without major overhead. This is a major part of the discussion of (C++'s model for type- and resource-safety).

See also:

Example
void f()
{
    int x = 0;
    int* p = &x;
    if (condition()) {
        int y = 0;
        p = &y;
    } // invalidates p
    *p = 42;            // BAD, p might be invalid if the branch was taken
}

To resolve the problem, either extend the lifetime of the object the pointer is intended to refer to, or shorten the lifetime of the pointer (move the dereference to before the pointed-to object's lifetime ends).

void f1()
{
    int x = 0;
    int* p = &x;
    int y = 0;
    if (condition()) {
        p = &y;
    }
    *p = 42;            // OK, p points to x or y and both are still in scope
}

Unfortunately, most invalid pointer problems are harder to spot and harder to fix.

Example
void f(int* p)
{
    int x = *p; // BAD: how do we know that p is valid?
}

There is a huge amount of such code. Most works -- after lots of testing -- but in isolation it is impossible to tell whether p could be the nullptr. Consequently, this is also a major source of errors. There are many approaches to dealing with this potential problem:

void f1(int* p) // deal with nullptr
{
    if (!p) {
        // deal with nullptr (allocate, return, throw, make p point to something, whatever
    }
    int x = *p;
}

There are two potential problems with testing for nullptr:

  • it is not always obvious what to do what to do if we find nullptr
  • the test can be redundant and/or relatively expensive
  • it is not obvious if the test is to protect against a violation or part of the required logic.
void f2(int* p) // state that p is not supposed to be nullptr
{
    assert(p);
    int x = *p;
}

This would carry a cost only when the assertion checking was enabled and would give a compiler/analyzer useful information. This would work even better if/when C++ gets direct support for contracts:

void f3(int* p) // state that p is not supposed to be nullptr
    [[expects: p]]
{
    int x = *p;
}

Alternatively, we could use gsl::not_null to ensure that p is not the nullptr.

void f(not_null<int*> p)
{
    int x = *p;
}

These remedies take care of nullptr only. Remember that there are other ways of getting an invalid pointer.

Example
void f(int* p)  // old code, doesn't use owner
{
    delete p;
}
void g()        // old code: uses naked new
{
    auto q = new int{7};
    f(q);
    int x = *q; // BAD: dereferences invalid pointer
}
Example
void f()
{
    vector<int> v(10);
    int* p = &v[5];
    v.push_back(99); // could reallocate v's elements
    int x = *p; // BAD: dereferences potentially invalid pointer
}
Enforcement

This rule is part of the (lifetime safety profile)

  • Flag a dereference of a pointer that points to an object that has gone out of scope
  • Flag a dereference of a pointer that might have been invalidated by assigning a nullptr
  • Flag a dereference of a pointer that might have been invalidated by a delete
  • Flag a dereference to a pointer to a container element that might have been invalidated by dereference

ES.stmt: Statements

Statements control the flow of control (except for function calls and exception throws, which are expressions).

ES.70: Prefer a switch-statement to an if-statement when there is a choice

Reason
  • Readability.
  • Efficiency: A switch compares against constants and is usually better optimized than a series of tests in an if-then-else chain.
  • A switch enables some heuristic consistency checking. For example, have all values of an enum been covered? If not, is there a default?
Example
void use(int n)
{
    switch (n) {   // good
    case 0:
        // ...
        break;
    case 7:
        // ...
        break;
    default:
        // ...
        break;
    }
}

rather than:

void use2(int n)
{
    if (n == 0)   // bad: if-then-else chain comparing against a set of constants
        // ...
    else if (n == 7)
        // ...
}
Enforcement

Flag if-then-else chains that check against constants (only).

ES.71: Prefer a range-for-statement to a for-statement when there is a choice

Reason

Readability. Error prevention. Efficiency.

Example
for (gsl::index i = 0; i < v.size(); ++i)   // bad
    cout << v[i] << '\n';
for (auto p = v.begin(); p != v.end(); ++p)   // bad
    cout << *p << '\n';
for (auto& x : v)    // OK
    cout << x << '\n';
for (gsl::index i = 1; i < v.size(); ++i) // touches two elements: can't be a range-for
    cout << v[i] + v[i - 1] << '\n';
for (gsl::index i = 0; i < v.size(); ++i) // possible side effect: can't be a range-for
    cout << f(v, &v[i]) << '\n';
for (gsl::index i = 0; i < v.size(); ++i) { // body messes with loop variable: can't be a range-for
    if (i % 2 != 0)
        cout << v[i] << '\n'; // output odd elements
}

A human or a good static analyzer might determine that there really isn't a side effect on v in f(v, &v[i]) so that the loop can be rewritten.

"Messing with the loop variable" in the body of a loop is typically best avoided.

Note

Don't use expensive copies of the loop variable of a range-for loop:

for (string s : vs) // ...

This will copy each elements of vs into s. Better:

for (string& s : vs) // ...

Better still, if the loop variable isn't modified or copied:

for (const string& s : vs) // ...
Enforcement

Look at loops, if a traditional loop just looks at each element of a sequence, and there are no side effects on what it does with the elements, rewrite the loop to a ranged-for loop.

ES.72: Prefer a for-statement to a while-statement when there is an obvious loop variable

Reason

Readability: the complete logic of the loop is visible "up front". The scope of the loop variable can be limited.

Example
for (gsl::index i = 0; i < vec.size(); i++) {
    // do work
}
Example, bad
int i = 0;
while (i < vec.size()) {
    // do work
    i++;
}
Enforcement

???

ES.73: Prefer a while-statement to a for-statement when there is no obvious loop variable

Reason

Readability.

Example
int events = 0;
for (; wait_for_event(); ++events) {  // bad, confusing
    // ...
}

The "event loop" is misleading because the events counter has nothing to do with the loop condition (wait_for_event()). Better

int events = 0;
while (wait_for_event()) {      // better
    ++events;
    // ...
}
Enforcement

Flag actions in for-initializers and for-increments that do not relate to the for-condition.

ES.74: Prefer to declare a loop variable in the initializer part of a for-statement

See (ES.6)

ES.75: Avoid do-statements

Reason

Readability, avoidance of errors. The termination condition is at the end (where it can be overlooked) and the condition is not checked the first time through.

Example
int x;
do {
    cin >> x;
    // ...
} while (x < 0);
Note

Yes, there are genuine examples where a do-statement is a clear statement of a solution, but also many bugs.

Enforcement

Flag do-statements.

ES.76: Avoid goto

Reason

Readability, avoidance of errors. There are better control structures for humans; goto is for machine generated code.

Exception

Breaking out of a nested loop. In that case, always jump forwards.

for (int i = 0; i < imax; ++i)
    for (int j = 0; j < jmax; ++j) {
        if (a[i][j] > elem_max) goto finished;
        // ...
    }
finished:
// ...
Example, bad

There is a fair amount of use of the C goto-exit idiom:

void f()
{
    // ...
        goto exit;
    // ...
        goto exit;
    // ...
exit:
    // ... common cleanup code ...
}

This is an ad-hoc simulation of destructors. Declare your resources with handles with destructors that clean up. If for some reason you cannot handle all cleanup with destructors for the variables used, consider gsl::finally() as a cleaner and more reliable alternative to goto exit

Enforcement
  • Flag goto. Better still flag all gotos that do not jump from a nested loop to the statement immediately after a nest of loops.

ES.77: Minimize the use of break and continue in loops

Reason

In a non-trivial loop body, it is easy to overlook a break or a continue.

A break in a loop has a dramatically different meaning than a break in a switch-statement (and you can have switch-statement in a loop and a loop in a switch-case).

Example
switch(x) {
case 1 :
    while (/* some condition */) {
        // ...
    break;
    } // Oops! break switch or break while intended?
case 2 :
    // ...
    break;
}
Alternative

Often, a loop that requires a break is a good candidate for a function (algorithm), in which case the break becomes a return.

//Original code: break inside loop
void use1()
{
    std::vector<T> vec = {/* initialized with some values */};
    T value;
    for (const T item : vec) {
        if (/* some condition*/) {
            value = item;
            break;
        }
    }
    /* then do something with value */
}
//BETTER: create a function and return inside loop
T search(const std::vector<T> &vec)
{
    for (const T &item : vec) {
        if (/* some condition*/) return item;
    }
    return T(); //default value
}
void use2()
{
    std::vector<T> vec = {/* initialized with some values */};
    T value = search(vec);
    /* then do something with value */
}

Often, a loop that uses continue can equivalently and as clearly be expressed by an if-statement.

for (int item : vec) {  // BAD
    if (item%2 == 0) continue;
    if (item == 5) continue;
    if (item > 10) continue;
    /* do something with item */
}
for (int item : vec) {  // GOOD
    if (item%2 != 0 && item != 5 && item <= 10) {
        /* do something with item */
    }
}
Note

If you really need to break out a loop, a break is typically better than alternatives such as ((modifying the loop variable) or a goto):

Enforcement

???

ES.78: Don't rely on implicit fallthrough in switch statements

Reason

Always end a non-empty case with a break. Accidentally leaving out a break is a fairly common bug. A deliberate fallthrough can be a maintenance hazard and should be rare and explicit.

Example
switch (eventType) {
case Information:
    update_status_bar();
    break;
case Warning:
    write_event_log();
    // Bad - implicit fallthrough
case Error:
    display_error_window();
    break;
}

Multiple case labels of a single statement is OK:

switch (x) {
case 'a':
case 'b':
case 'f':
    do_something(x);
    break;
}

Return statements in a case label are also OK:

switch (x) {
case 'a':
    return 1;
case 'b':
    return 2;
case 'c':
    return 3;
}
Exceptions

In rare cases if fallthrough is deemed appropriate, be explicit and use the [[fallthrough]] annotation:

switch (eventType) {
case Information:
    update_status_bar();
    break;
case Warning:
    write_event_log();
    [[fallthrough]];
case Error:
    display_error_window();
    break;
}
Note
Enforcement

Flag all implicit fallthroughs from non-empty cases.

ES.79: Use default to handle common cases (only)

Reason

Code clarity. Improved opportunities for error detection.

Example
enum E { a, b, c, d };
void f1(E x)
{
    switch (x) {
    case a:
        do_something();
        break;
    case b:
        do_something_else();
        break;
    default:
        take_the_default_action();
        break;
    }
}

Here it is clear that there is a default action and that cases a and b are special.

Example

But what if there is no default action and you mean to handle only specific cases? In that case, have an empty default or else it is impossible to know if you meant to handle all cases:

void f2(E x)
{
    switch (x) {
    case a:
        do_something();
        break;
    case b:
        do_something_else();
        break;
    default:
        // do nothing for the rest of the cases
        break;
    }
}

If you leave out the default, a maintainer and/or a compiler might reasonably assume that you intended to handle all cases:

void f2(E x)
{
    switch (x) {
    case a:
        do_something();
        break;
    case b:
    case c:
        do_something_else();
        break;
    }
}

Did you forget case d or deliberately leave it out? Forgetting a case typically happens when a case is added to an enumeration and the person doing so fails to add it to every switch over the enumerators.

Enforcement

Flag switch-statements over an enumeration that don't handle all enumerators and do not have a default. This might yield too many false positives in some code bases; if so, flag only switches that handle most but not all cases (that was the strategy of the very first C++ compiler).

ES.84: Don't try to declare a local variable with no name

Reason

There is no such thing. What looks to a human like a variable without a name is to the compiler a statement consisting of a temporary that immediately goes out of scope.

Example, bad
void f()
{
    lock<mutex>{mx};   // Bad
    // ...
}

This declares an unnamed lock object that immediately goes out of scope at the point of the semicolon. This is not an uncommon mistake. In particular, this particular example can lead to hard-to find race conditions.

Note

Unnamed function arguments are fine.

Enforcement

Flag statements that are just a temporary.

ES.85: Make empty statements visible

Reason

Readability.

Example
for (i = 0; i < max; ++i);   // BAD: the empty statement is easily overlooked
v[i] = f(v[i]);
for (auto x : v) {           // better
    // nothing
}
v[i] = f(v[i]);
Enforcement

Flag empty statements that are not blocks and don't contain comments.

ES.86: Avoid modifying loop control variables inside the body of raw for-loops

Reason

The loop control up front should enable correct reasoning about what is happening inside the loop. Modifying loop counters in both the iteration-expression and inside the body of the loop is a perennial source of surprises and bugs.

Example
for (int i = 0; i < 10; ++i) {
    // no updates to i -- ok
}
for (int i = 0; i < 10; ++i) {
    //
    if (/* something */) ++i; // BAD
    //
}
bool skip = false;
for (int i = 0; i < 10; ++i) {
    if (skip) { skip = false; continue; }
    //
    if (/* something */) skip = true;  // Better: using two variables for two concepts.
    //
}
Enforcement

Flag variables that are potentially updated (have a non-const use) in both the loop control iteration-expression and the loop body.

ES.87: Don't add redundant == or != to conditions

Reason

Doing so avoids verbosity and eliminates some opportunities for mistakes. Helps make style consistent and conventional.

Example

By definition, a condition in an if-statement, while-statement, or a for-statement selects between true and false. A numeric value is compared to 0 and a pointer value to nullptr.

// These all mean "if p is not nullptr"
if (p) { ... }            // good
if (p != 0) { ... }       // redundant !=0, bad: don't use 0 for pointers
if (p != nullptr) { ... } // redundant !=nullptr, not recommended

Often, if (p) is read as "if p is valid" which is a direct expression of the programmers intent, whereas if (p != nullptr) would be a long-winded workaround.

Example

This rule is especially useful when a declaration is used as a condition

if (auto pc = dynamic_cast<Circle>(ps)) { ... } // execute if ps points to a kind of Circle, good
if (auto pc = dynamic_cast<Circle>(ps); pc != nullptr) { ... } // not recommended
Example

Note that implicit conversions to bool are applied in conditions. For example:

for (string s; cin >> s; ) v.push_back(s);

This invokes istream's operator bool().

Note

Explicit comparison of an integer to 0 is in general not redundant. The reason is that (as opposed to pointers and Booleans) an integer often has more than two reasonable values. Furthermore 0 (zero) is often used to indicate success. Consequently, it is best to be specific about the comparison.

void f(int i)
{
    if (i)            // suspect
    // ...
    if (i == success) // possibly better
    // ...
}

Always remember that an integer can have more than two values.

Example, bad

It has been noted that

if(strcmp(p1, p2)) { ... }   // are the two C-style strings equal? (mistake!)

is a common beginners error. If you use C-style strings, you must know the <cstring> functions well. Being verbose and writing

if(strcmp(p1, p2) != 0) { ... }   // are the two C-style strings equal? (mistake!)

would not in itself save you.

Note

The opposite condition is most easily expressed using a negation:

// These all mean "if p is nullptr"
if (!p) { ... }           // good
if (p == 0) { ... }       // redundant == 0, bad: don't use 0 for pointers
if (p == nullptr) { ... } // redundant == nullptr, not recommended
Enforcement

Easy, just check for redundant use of != and == in conditions.

Arithmetic

ES.100: Don't mix signed and unsigned arithmetic

Reason

Avoid wrong results.

Example
int x = -3;
unsigned int y = 7;
cout << x - y << '\n';  // unsigned result, possibly 4294967286
cout << x + y << '\n';  // unsigned result: 4
cout << x * y << '\n';  // unsigned result, possibly 4294967275

It is harder to spot the problem in more realistic examples.

Note

Unfortunately, C++ uses signed integers for array subscripts and the standard library uses unsigned integers for container subscripts. This precludes consistency. Use gsl::index for subscripts; (see ES.107).

Enforcement
  • Compilers already know and sometimes warn.
  • (To avoid noise) Do not flag on a mixed signed/unsigned comparison where one of the arguments is sizeof or a call to container .size() and the other is ptrdiff_t.

ES.101: Use unsigned types for bit manipulation

Reason

Unsigned types support bit manipulation without surprises from sign bits.

Example
unsigned char x = 0b1010'1010;
unsigned char y = ~x;   // y == 0b0101'0101;
Note

Unsigned types can also be useful for modulo arithmetic. However, if you want modulo arithmetic add comments as necessary noting the reliance on wraparound behavior, as such code can be surprising for many programmers.

Enforcement
  • Just about impossible in general because of the use of unsigned subscripts in the standard library
  • ???

ES.102: Use signed types for arithmetic

Reason

Because most arithmetic is assumed to be signed; x - y yields a negative number when y > x except in the rare cases where you really want modulo arithmetic.

Example

Unsigned arithmetic can yield surprising results if you are not expecting it. This is even more true for mixed signed and unsigned arithmetic.

template<typename T, typename T2>
T subtract(T x, T2 y)
{
    return x - y;
}
void test()
{
    int s = 5;
    unsigned int us = 5;
    cout << subtract(s, 7) << '\n';       // -2
    cout << subtract(us, 7u) << '\n';     // 4294967294
    cout << subtract(s, 7u) << '\n';      // -2
    cout << subtract(us, 7) << '\n';      // 4294967294
    cout << subtract(s, us + 2) << '\n';  // -2
    cout << subtract(us, s + 2) << '\n';  // 4294967294
}

Here we have been very explicit about what's happening, but if you had seen us - (s + 2) or s += 2; ...; us - s, would you reliably have suspected that the result would print as 4294967294?

Exception

Use unsigned types if you really want modulo arithmetic - add comments as necessary noting the reliance on overflow behavior, as such code is going to be surprising for many programmers.

Example

The standard library uses unsigned types for subscripts. The built-in array uses signed types for subscripts. This makes surprises (and bugs) inevitable.

int a[10];
for (int i = 0; i < 10; ++i) a[i] = i;
vector<int> v(10);
// compares signed to unsigned; some compilers warn, but we should not
for (gsl::index i = 0; i < v.size(); ++i) v[i] = i;
int a2[-2];         // error: negative size
// OK, but the number of ints (4294967294) is so large that we should get an exception
vector<int> v2(-2);

Use gsl::index for subscripts; (see ES.107).

Enforcement
  • Flag mixed signed and unsigned arithmetic
  • Flag results of unsigned arithmetic assigned to or printed as signed.
  • Flag negative literals (e.g. -2) used as container subscripts.
  • (To avoid noise) Do not flag on a mixed signed/unsigned comparison where one of the arguments is sizeof or a call to container .size() and the other is ptrdiff_t.

ES.103: Don't overflow

Reason

Overflow usually makes your numeric algorithm meaningless. Incrementing a value beyond a maximum value can lead to memory corruption and undefined behavior.

Example, bad
int a[10];
a[10] = 7;   // bad, array bounds overflow
for (int n = 0; n <= 10; ++n)
    a[n] = 9;   // bad, array bounds overflow
Example, bad
int n = numeric_limits<int>::max();
int m = n + 1;   // bad, numeric overflow
Example, bad
int area(int h, int w) { return h * w; }
auto a = area(10'000'000, 100'000'000);   // bad, numeric overflow
Exception

Use unsigned types if you really want modulo arithmetic.

Alternative: For critical applications that can afford some overhead, use a range-checked integer and/or floating-point type.

Enforcement

???

ES.104: Don't underflow

Reason

Decrementing a value beyond a minimum value can lead to memory corruption and undefined behavior.

Example, bad
int a[10];
a[-2] = 7;   // bad
int n = 101;
while (n--)
    a[n - 1] = 9;   // bad (twice)
Exception

Use unsigned types if you really want modulo arithmetic.

Enforcement

???

ES.105: Don't divide by integer zero

Reason

The result is undefined and probably a crash.

Note

This also applies to %.

Example, bad
int divide(int a, int b)
{
    // BAD, should be checked (e.g., in a precondition)
    return a / b;
}
Example, good
int divide(int a, int b)
{
    // good, address via precondition (and replace with contracts once C++ gets them)
    Expects(b != 0);
    return a / b;
}
double divide(double a, double b)
{
    // good, address via using double instead
    return a / b;
}

Alternative: For critical applications that can afford some overhead, use a range-checked integer and/or floating-point type.

Enforcement
  • Flag division by an integral value that could be zero

ES.106: Don't try to avoid negative values by using unsigned

Reason

Choosing unsigned implies many changes to the usual behavior of integers, including modulo arithmetic, can suppress warnings related to overflow, and opens the door for errors related to signed/unsigned mixes. Using unsigned doesn't actually eliminate the possibility of negative values.

Example
unsigned int u1 = -2;   // Valid: the value of u1 is 4294967294
int i1 = -2;
unsigned int u2 = i1;   // Valid: the value of u2 is 4294967294
int i2 = u2;            // Valid: the value of i2 is -2

These problems with such (perfectly legal) constructs are hard to spot in real code and are the source of many real-world errors. Consider:

unsigned area(unsigned height, unsigned width) { return height*width; } // [(see also)](/interfaces#Ri-expects)
// ...
int height;
cin >> height;
auto a = area(height, 2);   // if the input is -2 a becomes 4294967292

Remember that -1 when assigned to an unsigned int becomes the largest unsigned int. Also, since unsigned arithmetic is modulo arithmetic the multiplication didn't overflow, it wrapped around.

Example
unsigned max = 100000;    // "accidental typo", I mean to say 10'000
unsigned short x = 100;
while (x < max) x += 100; // infinite loop

Had x been a signed short, we could have warned about the undefined behavior upon overflow.

Alternatives
  • use signed integers and check for x >= 0
  • use a positive integer type
  • use an integer subrange type
  • Assert(-1 < x)

For example

struct Positive {
    int val;
    Positive(int x) :val{x} { Assert(0 < x); }
    operator int() { return val; }
};
int f(Positive arg) { return arg; }
int r1 = f(2);
int r2 = f(-2);  // throws
Note

???

Enforcement

See ES.100 Enforcements.

ES.107: Don't use unsigned for subscripts, prefer gsl::index

Reason

To avoid signed/unsigned confusion. To enable better optimization. To enable better error detection. To avoid the pitfalls with auto and int.

Example, bad
vector<int> vec = /*...*/;
for (int i = 0; i < vec.size(); i += 2)                    // might not be big enough
    cout << vec[i] << '\n';
for (unsigned i = 0; i < vec.size(); i += 2)               // risk wraparound
    cout << vec[i] << '\n';
for (auto i = 0; i < vec.size(); i += 2)                   // might not be big enough
    cout << vec[i] << '\n';
for (vector<int>::size_type i = 0; i < vec.size(); i += 2) // verbose
    cout << vec[i] << '\n';
for (auto i = vec.size()-1; i >= 0; i -= 2)                // bug
    cout << vec[i] << '\n';
for (int i = vec.size()-1; i >= 0; i -= 2)                 // might not be big enough
    cout << vec[i] << '\n';
Example, good
vector<int> vec = /*...*/;
for (gsl::index i = 0; i < vec.size(); i += 2)             // ok
    cout << vec[i] << '\n';
for (gsl::index i = vec.size()-1; i >= 0; i -= 2)          // ok
    cout << vec[i] << '\n';
Note

The built-in array uses signed subscripts. The standard-library containers use unsigned subscripts. Thus, no perfect and fully compatible solution is possible (unless and until the standard-library containers change to use signed subscripts someday in the future). Given the known problems with unsigned and signed/unsigned mixtures, better stick to (signed) integers of a sufficient size, which is guaranteed by gsl::index.

Example
template<typename T>
struct My_container {
public:
    // ...
    T& operator[](gsl::index i);    // not unsigned
    // ...
};
Example
??? demonstrate improved code generation and potential for error detection ???
Alternatives

Alternatives for users

  • use algorithms
  • use range-for
  • use iterators/pointers
Enforcement
  • Very tricky as long as the standard-library containers get it wrong.
  • (To avoid noise) Do not flag on a mixed signed/unsigned comparison where one of the arguments is sizeof or a call to container .size() and the other is ptrdiff_t.