diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index a4f9240..7c6a10d 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -1,10 +1,6 @@ ---- -layout: default ---- - # C++ Core Guidelines -October 23, 2017 +November 27, 2017 Editors: @@ -683,7 +679,7 @@ You don't need to write error handlers for errors caught at compile time. for (Int i = 1; i; i <<= 1) ++bits; if (bits < 32) - cerr << "Int too small\n" + cerr << "Int too small\n"; This example fails to achieve what it is trying to achieve (because overflow is undefined) and should be replaced with a simple `static_assert`: @@ -1112,7 +1108,7 @@ Instead, we could use `vector`: ##### Note The standards library and the GSL are examples of this philosophy. -For example, instead of messing with the arrays, unions, cast, tricky lifetime issues, `gsl::owner`, etc. +For example, instead of messing with the arrays, unions, cast, tricky lifetime issues, `gsl::owner`, etc., that are needed to implement key abstractions, such as `vector`, `span`, `lock_guard`, and `future`, we use the libraries designed and implemented by people with more time and expertise than we usually have. Similarly, we can and should design and implement more specialized libraries, rather than leaving the users (often ourselves) @@ -1253,7 +1249,7 @@ Reporting through non-local variables (e.g., `errno`) is easily ignored. For exa // don't: no test of printf's return value fprintf(connection, "logging: %d %d %d\n", x, y, s); -What if the connection goes down so that no logging output is produced? See I.??. +What if the connection goes down so that no logging output is produced? See I.???. **Alternative**: Throw an exception. An exception cannot be ignored. @@ -2244,7 +2240,7 @@ So, we write a class Istream() { } Istream(zstring p) :owned{true}, inp{new ifstream{p}} {} // read from file Istream(zstring p, Opt) :owned{true}, inp{new istringstream{p}} {} // read from command line - ~Itream() { if (owned) delete inp; } + ~Istream() { if (owned) delete inp; } operator istream& () { return *inp; } private: bool owned = false; @@ -2318,7 +2314,7 @@ Other function rules: * [F.54: If you capture `this`, capture all variables explicitly (no default capture)](#Rf-this-capture) * [F.55: Don't use `va_arg` arguments](#F-varargs) -Functions have strong similarities to lambdas and function objects so see also Section ???. +Functions have strong similarities to lambdas and function objects so see also [C.lambdas: Function objects and lambdas](#SS-lambdas). ## F.def: Function definitions @@ -3569,7 +3565,7 @@ The language guarantees that a `T&` refers to an object, so that testing for `nu array w; // ... public: - wheel& get_wheel(size_t i) { Expects(i < 4); return w[i]; } + wheel& get_wheel(size_t i) { Expects(i < w.size()); return w[i]; } // ... }; @@ -3839,9 +3835,9 @@ It's confusing. Writing `[=]` in a member function appears to capture by value, // [=,this] and [&,this] are not much better, and confusing x = 42; - lambda(); // calls use(42); + lambda(); // calls use(0, 42); x = 43; - lambda(); // calls use(43); + lambda(); // calls use(0, 43); // ... @@ -6534,6 +6530,7 @@ Such as on an ABI (link) boundary. ##### Example struct Device { + virtual ~Device() = default; virtual void write(span outbuf) = 0; virtual void read(span inbuf) = 0; }; @@ -10670,560 +10667,6 @@ This is basically the way `printf` is implemented. * Flag `#include ` and `#include ` -## 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: // ... - case 7: // ... - } - } - -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 (int 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 (int i = 1; i < v.size(); ++i) // touches two elements: can't be a range-for - cout << v[i] + v[i - 1] << '\n'; - - for (int i = 0; i < v.size(); ++i) // possible side effect: can't be a range-for - cout << f(v, &v[i]) << '\n'; - - for (int i = 0; i < v.size(); ++i) { // body messes with loop variable: can't be a range-for - if (i % 2 == 0) - continue; // skip even elements - else - cout << v[i] << '\n'; - } - -A human or a good static analyzer may 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 (int 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 - -##### Reason - -Limit the loop variable visibility to the scope of the loop. -Avoid using the loop variable for other purposes after the loop. - -##### Example - - for (int i = 0; i < 100; ++i) { // GOOD: i var is visible only inside the loop - // ... - } - -##### 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](#Res-recycle) - -##### Example - - for (string s; cin >> s; ) { - cout << s << '\n'; - } - -##### Enforcement - -Warn when a variable modified inside the `for`-statement is declared outside the loop and not being used outside the loop. - -**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. - -### 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 `goto`s 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 - - ??? - -##### Alternative - -Often, a loop that requires a `break` is a good candidate for a function (algorithm), in which case the `break` becomes a `return`. - - ??? - -Often. a loop that uses `continue` can equivalently and as clearly be expressed by an `if`-statement. - - ??? - -##### Note - -If you really need to break out a loop, a `break` is typically better than alternatives such as [modifying the loop variable](#Res-loop-counter) or a [`goto`](#Res-goto): - - -##### Enforcement - -??? - -### ES.78: Always end a non-empty `case` with a `break` - -##### Reason - - Accidentally leaving out a `break` is a fairly common bug. - A deliberate fallthrough is a maintenance hazard. - -##### Example - - switch (eventType) { - case Information: - update_status_bar(); - break; - case Warning: - write_event_log(); - case Error: - display_error_window(); // Bad - break; - } - -It is easy to overlook the fallthrough. Be explicit: - - switch (eventType) { - case Information: - update_status_bar(); - break; - case Warning: - write_event_log(); - // fallthrough - case Error: - display_error_window(); // Bad - break; - } - -In C++17, use a `[[fallthrough]]` annotation: - - switch (eventType) { - case Information: - update_status_bar(); - break; - case Warning: - write_event_log(); - [[fallthrough]]; // C++17 - case Error: - display_error_window(); // Bad - break; - } - -##### Note - -Multiple case labels of a single statement is OK: - - switch (x) { - case 'a': - case 'b': - case 'f': - do_something(x); - break; - } - -##### Enforcement - -Flag all fallthroughs from non-empty `case`s. - -### 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 may 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 may yield too many false positives in some code bases; if so, flag only `switch`es 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. -To avoid unpleasant surprises. - -##### Example, bad - - void f() - { - lock{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. -There are exceedingly clever uses of this "idiom", but they are far rarer than the mistakes. - -##### 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 variable 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(ps)) { ... } // execute is ps points to a kind of Circle, good - - if (auto pc = dynamic_cast(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()`. - -##### 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 `` functions well. -Being verbose and writing - - if(strcmp(p1, p2) != 0) { ... } // are the two C-style strings equal? (mistake!) - -would not 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. - - ## ES.expr: Expressions Expressions manipulate values. @@ -11726,6 +11169,10 @@ are seriously overused as well as a major source of errors. If you feel the need for a lot of casts, there may be a fundamental design problem. +##### Exception + +Casting to `(void)` is the Standard-sanctioned way to turn off `[[nodiscard]]` warnings. If you are calling a function with a `[[nodiscard]]` return and you deliberately want to discard the 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), but if you still think it's appropriate and your code reviewer agrees, write `(void)` to turn off the warning. + ##### Alternatives Casts are widely (mis) used. Modern C++ has rules and constructs that eliminate the need for casts in many contexts, such as @@ -11736,7 +11183,7 @@ Casts are widely (mis) used. Modern C++ has rules and constructs that eliminate ##### Enforcement -* Force the elimination of C-style casts +* Force the elimination of C-style casts, except on a function with a `[[nodiscard]]` return * Warn if there are many functional style casts (there is an obvious problem in quantifying 'many') * The [type profile](#Pro-type-reinterpretcast) bans `reinterpret_cast`. * Warn against [identity casts](#Pro-type-identitycast) between pointer types, where the source and target types are the same (#Pro-type-identitycast) @@ -12380,7 +11827,7 @@ Unfortunately, most invalid pointer problems are harder to spot and harder to fi 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, it this is also a major source of errors. +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 @@ -12456,6 +11903,562 @@ This rule is part of the [lifetime profile](#Pro.lifetime) * Flag a dereference of a pointer that may have been invalidated by a `delete` * Flag a dereference to a pointer to a container element that may 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: // ... + case 7: // ... + } + } + +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 (int 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 (int i = 1; i < v.size(); ++i) // touches two elements: can't be a range-for + cout << v[i] + v[i - 1] << '\n'; + + for (int i = 0; i < v.size(); ++i) // possible side effect: can't be a range-for + cout << f(v, &v[i]) << '\n'; + + for (int i = 0; i < v.size(); ++i) { // body messes with loop variable: can't be a range-for + if (i % 2 == 0) + continue; // skip even elements + else + cout << v[i] << '\n'; + } + +A human or a good static analyzer may 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 (int 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 + +##### Reason + +Limit the loop variable visibility to the scope of the loop. +Avoid using the loop variable for other purposes after the loop. + +##### Example + + for (int i = 0; i < 100; ++i) { // GOOD: i var is visible only inside the loop + // ... + } + +##### 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](#Res-recycle) + +##### Example + + for (string s; cin >> s; ) { + cout << s << '\n'; + } + +##### Enforcement + +Warn when a variable modified inside the `for`-statement is declared outside the loop and not being used outside the loop. + +**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. + +### 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 `goto`s 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 + + ??? + +##### Alternative + +Often, a loop that requires a `break` is a good candidate for a function (algorithm), in which case the `break` becomes a `return`. + + ??? + +Often. a loop that uses `continue` can equivalently and as clearly be expressed by an `if`-statement. + + ??? + +##### Note + +If you really need to break out a loop, a `break` is typically better than alternatives such as [modifying the loop variable](#Res-loop-counter) or a [`goto`](#Res-goto): + + +##### Enforcement + +??? + +### ES.78: Always end a non-empty `case` with a `break` + +##### Reason + + Accidentally leaving out a `break` is a fairly common bug. + A deliberate fallthrough is a maintenance hazard. + +##### Example + + switch (eventType) { + case Information: + update_status_bar(); + break; + case Warning: + write_event_log(); + case Error: + display_error_window(); // Bad + break; + } + +It is easy to overlook the fallthrough. Be explicit: + + switch (eventType) { + case Information: + update_status_bar(); + break; + case Warning: + write_event_log(); + // fallthrough + case Error: + display_error_window(); // Bad + break; + } + +In C++17, use a `[[fallthrough]]` annotation: + + switch (eventType) { + case Information: + update_status_bar(); + break; + case Warning: + write_event_log(); + [[fallthrough]]; // C++17 + case Error: + display_error_window(); // Bad + break; + } + +##### Note + +Multiple case labels of a single statement is OK: + + switch (x) { + case 'a': + case 'b': + case 'f': + do_something(x); + break; + } + +##### Enforcement + +Flag all fallthroughs from non-empty `case`s. + +### 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 may 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 may yield too many false positives in some code bases; if so, flag only `switch`es 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. +To avoid unpleasant surprises. + +##### Example, bad + + void f() + { + lock{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. +There are exceedingly clever uses of this "idiom", but they are far rarer than the mistakes. + +##### 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 variable 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(ps)) { ... } // execute is ps points to a kind of Circle, good + + if (auto pc = dynamic_cast(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()`. + +##### 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 `` functions well. +Being verbose and writing + + if(strcmp(p1, p2) != 0) { ... } // are the two C-style strings equal? (mistake!) + +would not 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 @@ -13399,7 +13402,8 @@ Application concepts are easier to reason about. void some_fun() { std::string msg, msg2; - std::thread publisher([&] { msg = "Hello"; }); // bad (less expressive and more error-prone) + std::thread publisher([&] { msg = "Hello"; }); // bad: less expressive + // and more error-prone auto pubtask = std::async([&] { msg2 = "Hello"; }); // OK // ... publisher.join(); @@ -17983,7 +17987,7 @@ Source file rule summary: * [SF.4: Include `.h` files before other declarations in a file](#Rs-include-order) * [SF.5: A `.cpp` file must include the `.h` file(s) that defines its interface](#Rs-consistency) * [SF.6: Use `using namespace` directives for transition, for foundation libraries (such as `std`), or within a local scope (only)](#Rs-using) -* [SF.7: Don't write `using namespace` in a header file](#Rs-using-directive) +* [SF.7: Don't write `using namespace` at global scope in a header file](#Rs-using-directive) * [SF.8: Use `#include` guards for all `.h` files](#Rs-guards) * [SF.9: Avoid cyclic dependencies among source files](#Rs-cycles) * [SF.10: Avoid dependencies on implicitly `#include`d names](#Rs-implicit) @@ -18241,11 +18245,11 @@ and M functions each containing a `using namespace X`with N lines of code in tot Flag multiple `using namespace` directives for different namespaces in a single source file. -### SF.7: Don't write `using namespace` in a header file +### SF.7: Don't write `using namespace` at global scope in a header file ##### Reason -Doing so takes away an `#include`r's ability to effectively disambiguate and to use alternatives. +Doing so takes away an `#include`r's ability to effectively disambiguate and to use alternatives. It also makes `#include`d headers order-dependent as they may have different meaning when included in different orders. ##### Example @@ -19057,7 +19061,7 @@ Synchronizing `iostreams` with `printf-style` I/O can be costly. ### SL.io.50: Avoid `endl` -### Reason +##### Reason The `endl` manipulator is mostly equivalent to `'\n'` and `"\n"`; as most commonly used it simply slows down output by doing redundant `flush()`s.