diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 6906160..f65074b 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -87,7 +87,6 @@ Definitions of terms used to express and discuss the rules, that are not languag * resource * exception guarantee - # Abstract This document is a set of guidelines for using C++ well. @@ -127,7 +126,6 @@ We plan to build tools for that and hope others will too. Comments and suggestions for improvements are most welcome. We plan to modify and extend this document as our understanding improves and the language and the set of available libraries improve. - # In: Introduction This is a set of core guidelines for modern C++, C++14, and taking likely future enhancements and taking ISO Technical Specifications (TSs) into account. @@ -142,12 +140,10 @@ Introduction summary: * [In.struct: The structure of this document](#SS-struct) * [In.sec: Major sections](#SS-sec) - ## In.target: Target readership All C++ programmers. This includes [programmers who might consider C](#S-cpl). - ## In.aims: Aims The purpose of this document is to help developers to adopt modern C++ (C++11, C++14, and soon C++17) and to achieve a more uniform style across code bases. @@ -229,7 +225,6 @@ The rules are not value-neutral. They are meant to make code simpler and more correct/safer than most existing C++ code, without loss of performance. They are meant to inhibit perfectly valid C++ code that correlates with errors, spurious complexity, and poor performance. - ## In.force: Enforcement Rules with no enforcement are unmanageable for large code bases. @@ -260,7 +255,6 @@ For a start, we have a few profiles corresponding to common needs (desires, idea The profiles are intended to be used by tools, but also serve as an aid to the human reader. We do not limit our comment in the **Enforcement** sections to things we know how to enforce; some comments are mere wishes that might inspire some tool builder. - ## In.struct: The structure of this document Each rule (guideline, suggestion) can have several parts: @@ -291,7 +285,6 @@ This is not a language manual. It is meant to be helpful, rather than complete, fully accurate on technical details, or a guide to existing code. Recommended information sources can be found in [the references](#S-references). - ## In.sec: Major sections * [P: Philosophy](#S-philosophy) @@ -327,7 +320,6 @@ These sections are not orthogonal. Each section (e.g., "P" for "Philosophy") and each subsection (e.g., "C.hier" for "Class Hierarchies (OOP)") have an abbreviation for ease of searching and reference. The main section abbreviations are also used in rule numbers (e.g., "C.11" for "Make concrete types regular"). - # P: Philosophy The rules in this section are very general. @@ -357,44 +349,44 @@ What is expressed in code has a defined semantics and can (in principle) be chec ##### Example - class Date { - // ... - public: - Month month() const; // do - int month(); // don't - // ... - }; + class Date { + // ... + public: + Month month() const; // do + int month(); // don't + // ... + }; The first declaration of `month` is explicit about returning a `Month` and about not modifying the state of the `Date` object. The second version leaves the reader guessing and opens more possibilities for uncaught bugs. ##### Example - void do_something(vector& v) - { - string val; - cin>>val; - // ... - int index = 0; // bad - for(int i=0; i& v) + { + string val; + cin>>val; + // ... + int index = 0; // bad + for(int i=0; i& v) - { - string val; - cin>>val; - // ... - auto p = find(v, val); // better - // ... - } + void do_something(vector& v) + { + string val; + cin>>val; + // ... + auto p = find(v, val); // better + // ... + } A well-designed library expresses intent (what is to be done, rather than just how something is being done) far better than direct use of language features. @@ -404,16 +396,16 @@ Any programmer using these guidelines should know the [Guidelines Support Librar ##### Example - change_speed(double s); // bad: what does s signify? - // ... - change_speed(2.3); + change_speed(double s); // bad: what does s signify? + // ... + change_speed(2.3); A better approach is to be explicit about the meaning of the double (new speed or delta on old speed?) and the unit used: - change_speed(Speed s); // better: the meaning of s is specified - // ... - change_speed(2.3); // error: no unit - change_speed(23m/10s); // meters per second + change_speed(Speed s); // better: the meaning of s is specified + // ... + change_speed(2.3); // error: no unit + change_speed(23m/10s); // meters per second We could have accepted a plain (unit-less) `double` as a delta, but that would have been error-prone. If we wanted both absolute speed and deltas, we would have defined a `Delta` type. @@ -455,25 +447,25 @@ Use an up-to-date C++ compiler (currently C++11 or C++14) with a set of options ##### Example - int i = 0; - while (i=4); // do: compile-time check + void initializer(Int x) + // Int is an alias used for integers + { + static_assert(sizeof(Int)>=4); // do: compile-time check - int bits = 0; // don't: avoidable code - for (Int i = 1; i; i<<=1) - ++bits; - if (bits<32) - cerr << "Int too small\n"; + int bits = 0; // don't: avoidable code + for (Int i = 1; i; i<<=1) + ++bits; + if (bits<32) + cerr << "Int too small\n"; - // ... - } + // ... + } ##### Example don't - void read(int* p, int n); // read max n integers into *p + void read(int* p, int n); // read max n integers into *p ##### Example - void read(array_view r); // read into the range of integers r + void read(array_view r); // read into the range of integers r **Alternative formulation**: Don't postpone to run time what can be done well at compile time. @@ -589,12 +581,12 @@ Ideally we catch all errors (that are not errors in the programmer's logic) at e ##### Example, bad - extern void f(int* p); // separately compiled, possibly dynamically loaded + extern void f(int* p); // separately compiled, possibly dynamically loaded - void g(int n) - { - f(new int[n]); // bad: the number of elements is not passed to f() - } + void g(int n) + { + f(new int[n]); // bad: the number of elements is not passed to f() + } Here, a crucial bit of information (the number of elements) has been so thoroughly "obscured" that static analysis is probably rendered infeasible and dynamic checking can be very difficult when `f()` is part of an ABI so that we cannot "instrument" that pointer. We could embed helpful information into the free store, but that requires global changes to a system and maybe to the compiler. What we have here is a design that makes error detection very hard. @@ -602,12 +594,12 @@ Here, a crucial bit of information (the number of elements) has been so thorough We can of course pass the number of elements along with the pointer: - extern void f2(int* p, int n); // separately compiled, possibly dynamically loaded + extern void f2(int* p, int n); // separately compiled, possibly dynamically loaded - void g2(int n) - { - f2(new int[n], m); // bad: the wrong number of elements can be passed to f() - } + void g2(int n) + { + f2(new int[n], m); // bad: the wrong number of elements can be passed to f() + } Passing the number of elements as an argument is better (and far more common) that just passing the pointer and relying on some (unstated) convention for knowing or discovering the number of elements. However (as shown), a simple typo can introduce a serious error. The connection between the two arguments of `f2()` is conventional, rather than explicit. @@ -617,26 +609,26 @@ Also, it is implicit that `f2()` is supposed to `delete` its argument (or did th The standard library resource management pointers fail to pass the size when they point to an object: - extern void f3(unique_ptr, int n); // separately compiled, possibly dynamically loaded + extern void f3(unique_ptr, int n); // separately compiled, possibly dynamically loaded - void g3(int n) - { - f3(make_unique(n), m); // bad: pass ownership and size separately - } + void g3(int n) + { + f3(make_unique(n), m); // bad: pass ownership and size separately + } ##### Example We need to pass the pointer and the number of elements as an integral object: - extern void f4(vector&); // separately compiled, possibly dynamically loaded - extern void f4(array_view); // separately compiled, possibly dynamically loaded + extern void f4(vector&); // separately compiled, possibly dynamically loaded + extern void f4(array_view); // separately compiled, possibly dynamically loaded - void g3(int n) - { - vector v(n); - f4(v); // pass a reference, retain ownership - f4(array_view{v}); // pass a view, retain ownership - } + void g3(int n) + { + vector v(n); + f4(v); // pass a reference, retain ownership + f4(array_view{v}); // pass a view, retain ownership + } This design carries the number of elements along as an integral part of an object, so that errors are unlikely and dynamic (run-time) checking is always feasible, if not always affordable. @@ -644,32 +636,32 @@ This design carries the number of elements along as an integral part of an objec How do we transfer both ownership and all information needed for validating use? - vector f5(int n) // OK: move - { - vector v(n); - // ... initialize v ... - return v; - } + vector f5(int n) // OK: move + { + vector v(n); + // ... initialize v ... + return v; + } - unique_ptr f6(int n) // bad: loses n - { - auto p = make_unique(n); - // ... initialize *p ... - return p; - } + unique_ptr f6(int n) // bad: loses n + { + auto p = make_unique(n); + // ... initialize *p ... + return p; + } - owner f7(int n) // bad: loses n and we might forget to delete - { - owner p = new int[n]; - // ... initialize *p ... - return p; - } + owner f7(int n) // bad: loses n and we might forget to delete + { + owner p = new int[n]; + // ... initialize *p ... + return p; + } ##### Example - * ??? - * show how possible checks are avoided by interfaces that pass polymorphic base classes around, when they actually know what they need? - Or strings as "free-style" options +* ??? +* show how possible checks are avoided by interfaces that pass polymorphic base classes around, when they actually know what they need? + Or strings as "free-style" options ##### Enforcement @@ -685,73 +677,73 @@ Avoid errors leading to (possibly unrecognized) wrong results. ##### Example - void increment1(int* p, int n) // bad: error prone - { - for (int i=0; i p) - { - for (int& x : p) ++x; - } + void increment2(array_view p) + { + for (int& x : p) ++x; + } - void use2(int m) - { - const int n = 10; - int a[n] = {}; - // ... - increment2({a, m}); // maybe typo, maybe m<=n is supposed - // ... - } + void use2(int m) + { + const int n = 10; + int a[n] = {}; + // ... + increment2({a, m}); // maybe typo, maybe m<=n is supposed + // ... + } Now, `m<=n` can be checked at the point of call (early) rather than later. If all we had was a typo so that we meant to use `n` as the bound, the code could be further simplified (eliminating the possibility of an error): - void use3(int m) - { - const int n = 10; - int a[n] = {}; - // ... - increment2(a); // the number of elements of a need not be repeated - // ... - } + void use3(int m) + { + const int n = 10; + int a[n] = {}; + // ... + increment2(a); // the number of elements of a need not be repeated + // ... + } ##### Example, bad Don't repeatedly check the same value. Don't pass structured data as strings: - Date read_date(istream& is); // read date from istream + Date read_date(istream& is); // read date from istream - Date extract_date(const string& s); // extract date from string + Date extract_date(const string& s); // extract date from string - void user1(const string& date) // manipulate date - { - auto d = extract_date(date); - // ... - } + void user1(const string& date) // manipulate date + { + auto d = extract_date(date); + // ... + } - void user2() - { - Date d = read_date(cin); - // ... - user1(d.to_string()); - // ... - } + void user2() + { + Date d = read_date(cin); + // ... + user1(d.to_string()); + // ... + } The date is validated twice (by the `Date` constructor) and passed as a character string (unstructured data). @@ -799,24 +791,24 @@ The physical law for a jet (`e*e < x*x + y*y + z*z`) is not an invariant because ##### Example, bad - void f(char* name) - { - FILE* input = fopen(name, "r"); - // ... - if (something) return; // bad: if something==true, a file handle is leaked - // ... - fclose(input); - } + void f(char* name) + { + FILE* input = fopen(name, "r"); + // ... + if (something) return; // bad: if something==true, a file handle is leaked + // ... + fclose(input); + } Prefer [RAII](#Rr-raii): - void f(char* name) - { - ifstream input {name}; - // ... - if (something) return; // OK: no leak - // ... - } + void f(char* name) + { + ifstream input {name}; + // ... + if (something) return; // OK: no leak + // ... + } **See also**: [The resource management section](#S-resource) @@ -842,37 +834,37 @@ Time and space that you spend well to achieve a goal (e.g., speed of development ??? more and better suggestions for gratuitous waste welcome ??? - struct X { - char ch; - int i; - string s; - char ch2; + struct X { + char ch; + int i; + string s; + char ch2; - X& operator=(const X& a); - X(const X&); - }; + X& operator=(const X& a); + X(const X&); + }; - X waste(const char* p) - { - if (p==nullptr) throw Nullptr_error{}; - int n = strlen(p); - auto buf = new char[n]; - if (buf==nullptr) throw Allocation_error{}; - for (int i = 0; i=0); /* ... */ } + double sqrt(double x) { Expects(x>=0); /* ... */ } Ideally, that `Expects(x>=0)` should be part of the interface of `sqrt()` but that's not easily done. For now, we place it in the definition (function body). @@ -1154,12 +1146,12 @@ We don't need to mention it for each member function. ##### Example - int area(int height, int width) - { - Expects(height>0 && width>0); // good - if (height<=0 || width<=0) my_error(); // obscure - // ... - } + int area(int height, int width) + { + Expects(height>0 && width>0); // good + if (height<=0 || width<=0) my_error(); // obscure + // ... + } ##### Note @@ -1187,40 +1179,40 @@ Preconditions should be part of the interface rather than part of the implementa Consider - int area(int height, int width) { return height*width; } // bad + int area(int height, int width) { return height*width; } // bad Here, we (incautiously) left out the precondition specification, so it is not explicit that height and width must be positive. We also left out the postcondition specification, so it is not obvious that the algorithm (`height*width`) is wrong for areas larger than the largest integer. Overflow can happen. Consider using: - int area(int height, int width) - { - auto res = height*width; - Ensures(res>0); - return res; - } + int area(int height, int width) + { + auto res = height*width; + Ensures(res>0); + return res; + } ##### Example, bad Consider a famous security bug - void f() // problematic - { - char buffer[MAX]; - // ... - memset(buffer, 0, MAX); - } + void f() // problematic + { + char buffer[MAX]; + // ... + memset(buffer, 0, MAX); + } There was no postcondition stating that the buffer should be cleared and the optimizer eliminated the apparently redundant `memset()` call: - void f() // better - { - char buffer[MAX]; - // ... - memset(buffer, 0, MAX); - Ensures(buffer[0]==0); - } + void f() // better + { + char buffer[MAX]; + // ... + memset(buffer, 0, MAX); + Ensures(buffer[0]==0); + } ##### Note @@ -1234,31 +1226,31 @@ Postconditions are especially important when they relate to something that is no Consider a function that manipulates a `Record`, using a `mutex` to avoid race conditions: - mutex m; + mutex m; - void manipulate(Record& r) // don't - { - m.lock(); - // ... no m.unlock() ... - } + void manipulate(Record& r) // don't + { + m.lock(); + // ... no m.unlock() ... + } Here, we "forgot" to state that the `mutex` should be released, so we don't know if the failure to ensure release of the `mutex` was a bug or a feature. Stating the postcondition would have made it clear: - void manipulate(Record& r) // better: hold the mutex m while and only while manipulating r - { - m.lock(); - // ... no m.unlock() ... - } + void manipulate(Record& r) // better: hold the mutex m while and only while manipulating r + { + m.lock(); + // ... no m.unlock() ... + } The bug is now obvious. Better still, use [RAII](#Rr-raii) to ensure that the postcondition ("the lock must be released") is enforced in code: - void manipulate(Record& r) // best - { - lock_guard _ {m}; - // ... - } + void manipulate(Record& r) // best + { + lock_guard _ {m}; + // ... + } ##### Note @@ -1278,13 +1270,13 @@ Postconditions related only to internal state belongs in the definition/implemen ##### Example - void f() - { - char buffer[MAX]; - // ... - memset(buffer, 0, MAX); - Ensures(buffer[0]==0); - } + void f() + { + char buffer[MAX]; + // ... + memset(buffer, 0, MAX); + Ensures(buffer[0]==0); + } ##### Note @@ -1308,12 +1300,12 @@ Ideally, that `Ensures` should be part of the interface, but that's not easily d Use the ISO Concepts TS style of requirements specification. For example: - template - // requires InputIterator && EqualityComparable>, Val> - Iter find(Iter first, Iter last, Val v) - { - // ... - } + template + // requires InputIterator && EqualityComparable>, Val> + Iter find(Iter first, Iter last, Val v) + { + // ... + } ##### Note @@ -1334,11 +1326,10 @@ This is a major source of errors. ##### Example - int printf(const char* ...); // bad: return negative number if output fails - - template - explicit thread(F&& f, Args&&... args); // good: throw system_error if unable to start the new thread + int printf(const char* ...); // bad: return negative number if output fails + template + explicit thread(F&& f, Args&&... args); // good: throw system_error if unable to start the new thread ##### Note: What is an error? @@ -1353,13 +1344,13 @@ However, if failing to make a connection is considered an error, then a failure **Alternative**: If you can't use exceptions (e.g. because your code is full of old-style raw-pointer use or because there are hard-real-time constraints), consider using a style that returns a pair of values: - int val; - int error_code; - tie(val, error_code) = do_something(); - if (error_code==0) { - // ... handle the error or exit ... - } - // ... use val ... + int val; + int error_code; + tie(val, error_code) = do_something(); + if (error_code==0) { + // ... handle the error or exit ... + } + // ... use val ... ##### Note @@ -1387,22 +1378,22 @@ We don't consider "performance" a valid reason not to use exceptions. Consider - X* compute(args) // don't - { - X* res = new X{}; - // ... - return res; - } + X* compute(args) // don't + { + X* res = new X{}; + // ... + return res; + } Who deletes the returned `X`? The problem would be harder to spot if compute returned a reference. Consider returning the result by value (use move semantics if the result is large): - vector compute(args) // good - { - vector res(10000); - // ... - return res; - } + vector compute(args) // good + { + vector res(10000); + // ... + return res; + } **Alternative**: Pass ownership using a "smart pointer", such as `unique_ptr` (for exclusive ownership) and `shared_ptr` (for shared ownership). However that is less elegant and less efficient unless reference semantics are needed. @@ -1410,12 +1401,12 @@ However that is less elegant and less efficient unless reference semantics are n **Alternative**: Sometimes older code can't be modified because of ABI compatibility requirements or lack of resources. In that case, mark owning pointers using `owner`: - owner compute(args) // It is now clear that ownership is transferred - { - owner res = new X{}; - // ... - return res; - } + owner compute(args) // It is now clear that ownership is transferred + { + owner res = new X{}; + // ... + return res; + } This tells analysis tools that `res` is an owner. That is, its value must be `delete`d or transferred to another owner, as is done here by the `return`. @@ -1436,7 +1427,6 @@ Every object passed as a raw pointer (or iterator) is assumed to be owned by the * (Simple) Warn on failure to either `reset` or explicitly `delete` an `owner` pointer on every code path. * (Simple) Warn if the return value of `new` or a function call with return value of pointer type is assigned to a raw pointer. - ### I.12: Declare a pointer that must not be null as `not_null` ##### Reason @@ -1445,13 +1435,13 @@ Every object passed as a raw pointer (or iterator) is assumed to be owned by the ##### Example - int length(const char* p); // it is not clear whether length(nullptr) is valid + int length(const char* p); // it is not clear whether length(nullptr) is valid - length(nullptr); // OK? + length(nullptr); // OK? - int length(not_null p); // better: we can assume that p cannot be nullptr + int length(not_null p); // better: we can assume that p cannot be nullptr - int length(const char* p); // we must assume that p can be nullptr + int length(const char* p); // we must assume that p can be nullptr By stating the intent in source, implementers and tools can provide better diagnostics, such as finding some classes of errors through static analysis, and perform optimizations, such as removing branches and null tests. @@ -1459,8 +1449,8 @@ By stating the intent in source, implementers and tools can provide better diagn The assumption that the pointer to `char` pointed to a C-style string (a zero-terminated string of characters) was still implicit, and a potential source of confusion and errors. Use `zstring` in preference to `const char*`. - int length(not_null p); // we can assume that p cannot be nullptr - // we can assume that p points to a zero-terminated array of characters + int length(not_null p); // we can assume that p cannot be nullptr + // we can assume that p points to a zero-terminated array of characters Note: `length()` is, of course, `std::strlen()` in disguise. @@ -1479,7 +1469,7 @@ Note: `length()` is, of course, `std::strlen()` in disguise. Consider - void copy_n(const T* p, T* q, int n); // copy from [p:p+n) to [q:q+n) + void copy_n(const T* p, T* q, int n); // copy from [p:p+n) to [q:q+n) What if there are fewer than `n` elements in the array pointed to by `q`? Then, we overwrite some probably unrelated memory. What if there are fewer than `n` elements in the array pointed to by `p`? Then, we read some probably unrelated memory. @@ -1487,29 +1477,29 @@ Either is undefined behavior and a potentially very nasty bug. **Alternative**: Consider using explicit ranges, - void copy(array_view r, array_view r2); // copy r to r2 + void copy(array_view r, array_view r2); // copy r to r2 ##### Example, bad Consider - void draw(Shape* p, int n); // poor interface; poor code - Circle arr[10]; - // ... - draw(arr, 10); + void draw(Shape* p, int n); // poor interface; poor code + Circle arr[10]; + // ... + draw(arr, 10); Passing `10` as the `n` argument may be a mistake: the most common convention is to assume [`0`:`n`) but that is nowhere stated. Worse is that the call of `draw()` compiled at all: there was an implicit conversion from array to pointer (array decay) and then another implicit conversion from `Circle` to `Shape`. There is no way that `draw()` can safely iterate through that array: it has no way of knowing the size of the elements. **Alternative**: Use a support class that ensures that the number of elements is correct and prevents dangerous implicit conversions. For example: - void draw2(array_view); - Circle arr[10]; - // ... - draw2(array_view(arr)); // deduce the number of elements - draw2(arr); // deduce the element type and array size + void draw2(array_view); + Circle arr[10]; + // ... + draw2(array_view(arr)); // deduce the number of elements + draw2(arr); // deduce the element type and array size - void draw3(array_view); - draw3(arr); // error: cannot convert Circle[10] to array_view + void draw3(array_view); + draw3(arr); // error: cannot convert Circle[10] to array_view This `draw2()` passes the same amount of information to `draw()`, but makes the fact that it is supposed to be a range of `Circle`s explicit. See ???. @@ -1530,24 +1520,24 @@ This `draw2()` passes the same amount of information to `draw()`, but makes the The standard-library `merge()` is at the limit of what we can comfortably handle - template - OutputIterator merge(InputIterator1 first1, InputIterator1 last1, - InputIterator2 first2, InputIterator2 last2, - OutputIterator result, Compare comp); + template + OutputIterator merge(InputIterator1 first1, InputIterator1 last1, + InputIterator2 first2, InputIterator2 last2, + OutputIterator result, Compare comp); Here, we have four template arguments and six function arguments. To simplify the most frequent and simplest uses, the comparison argument can be defaulted to `<`: - template - OutputIterator merge(InputIterator1 first1, InputIterator1 last1, - InputIterator2 first2, InputIterator2 last2, - OutputIterator result); + template + OutputIterator merge(InputIterator1 first1, InputIterator1 last1, + InputIterator2 first2, InputIterator2 last2, + OutputIterator result); This doesn't reduce the total complexity, but it reduces the surface complexity presented to many users. To really reduce the number of arguments, we need to bundle the arguments into higher-level abstractions: - template - OutputIterator merge(InputRange1 r1, InputRange2 r2, OutputIterator result); + template + OutputIterator merge(InputRange1 r1, InputRange2 r2, OutputIterator result); Grouping arguments into "bundles" is a general technique to reduce the number of arguments and to increase the opportunities for checking. @@ -1575,23 +1565,23 @@ There are functions that are best expressed with four individual arguments, but Consider - void copy_n(T* p, T* q, int n); // copy from [p:p+n) to [q:q+n) + void copy_n(T* p, T* q, int n); // copy from [p:p+n) to [q:q+n) This is a nasty variant of a K&R C-style interface. It is easy to reverse the "to" and "from" arguments. Use `const` for the "from" argument: - void copy_n(const T* p, T* q, int n); // copy from [p:p+n) to [q:q+n) + void copy_n(const T* p, T* q, int n); // copy from [p:p+n) to [q:q+n) ##### Example If the order of the parameters is not important, there is no problem: - int max(int a, int b); + int max(int a, int b); **Alternative**: Don't pass arrays as pointers, pass an object representing a range (e.g., an `array_view`): - void copy_n(array_view p, array_view q); // copy from p to q + void copy_n(array_view p, array_view q); // copy from p to q ##### Enforcement @@ -1607,28 +1597,28 @@ If the order of the parameters is not important, there is no problem: You just knew that `Shape` would turn up somewhere :-) - class Shape { // bad: interface class loaded with data - public: - Point center() { return c; } - virtual void draw(); - virtual void rotate(int); - // ... - private: - Point c; - vector outline; - Color col; - }; + class Shape { // bad: interface class loaded with data + public: + Point center() { return c; } + virtual void draw(); + virtual void rotate(int); + // ... + private: + Point c; + vector outline; + Color col; + }; This will force every derived class to compute a center -- even if that's non-trivial and the center is never used. Similarly, not every `Shape` has a `Color`, and many `Shape`s are best represented without an outline defined as a sequence of `Point`s. Abstract classes were invented to discourage users from writing such classes: - class Shape { // better: Shape is a pure interface - public: - virtual Point center() =0; // pure virtual function - virtual void draw() =0; - virtual void rotate(int) =0; - // ... - // ... no data members ... - }; + class Shape { // better: Shape is a pure interface + public: + virtual Point center() =0; // pure virtual function + virtual void draw() =0; + virtual void rotate(int) =0; + // ... + // ... no data members ... + }; ##### Enforcement @@ -1706,12 +1696,10 @@ Other function rules: Functions have strong similarities to lambdas and function objects so see also Section ???. - ## F.def: Function definitions A function definition is a function declaration that also specifies the function's implementation, the function body. - ### F.1: "Package" meaningful operations as carefully named functions ##### Reason @@ -1721,14 +1709,14 @@ If something is a well-specified action, separate it out from its surrounding co ##### Example, don't - void read_and_print(istream& is) // read and print an int - { - int x; - if (is>>x) - cout << "the int is " << x << '\n'; - else - cerr << "no int on input\n"; - } + void read_and_print(istream& is) // read and print an int + { + int x; + if (is>>x) + cout << "the int is " << x << '\n'; + else + cerr << "no int on input\n"; + } Almost everything is wrong with `read_and_print`. It reads, it writes (to a fixed `ostream`), it writes error messages (to a fixed `ostream`), it handles only `int`s. @@ -1743,14 +1731,14 @@ give it a name by assigning it to a (usually non-local) variable. ##### Example - sort(a, b, [](T x, T y) { return x.valid() && y.valid() && x.value() F.2: A function should perform a single logical operation ##### Reason @@ -1775,49 +1762,49 @@ Similarly, lambdas used as callback arguments are sometimes non-trivial, yet unl Consider - void read_and_print() // bad - { - int x; - cin >> x; - // check for errors - cout << x << "\n"; - } + void read_and_print() // bad + { + int x; + cin >> x; + // check for errors + cout << x << "\n"; + } This is a monolith that is tied to a specific input and will never find a another (different) use. Instead, break functions up into suitable logical parts and parameterize: - int read(istream& is) // better - { - int x; - is >> x; - // check for errors - return x; - } + int read(istream& is) // better + { + int x; + is >> x; + // check for errors + return x; + } - void print(ostream& os, int x) - { - os << x << "\n"; - } + void print(ostream& os, int x) + { + os << x << "\n"; + } These can now be combined where needed: - void read_and_print() - { - auto x = read(cin); - print(cout, x); - } + void read_and_print() + { + auto x = read(cin); + print(cout, x); + } If there was a need, we could further templatize `read()` and `print()` on the data type, the I/O mechanism, etc. Example: - auto read = [](auto& input, auto& value) // better - { - input >> value; - // check for errors - }; + auto read = [](auto& input, auto& value) // better + { + input >> value; + // check for errors + }; - auto print(auto& output, const auto& value) - { - output << value << "\n"; - } + auto print(auto& output, const auto& value) + { + output << value << "\n"; + } ##### Enforcement @@ -1836,9 +1823,10 @@ Functions with complex control structures are more likely to be long and more li Consider - double simpleFunc(double val, int flag1, int flag2) - // simpleFunc: takes a value and calculates the expected ASIC output, given the two mode flags. - { + double simpleFunc(double val, int flag1, int flag2) + // simpleFunc: takes a value and calculates the expected ASIC output, given the two mode flags. + { + double intermediate; if (flag1 > 0) { intermediate = func1(val); @@ -1868,19 +1856,20 @@ Yes, it breaks other rules also. We can refactor: - double func1_muon(double val, int flag) - { - // ??? - } + double func1_muon(double val, int flag) + { + // ??? + } - double funct1_tau(double val, int flag1, int flag2) - { - // ??? - } + double funct1_tau(double val, int flag1, int flag2) + { + // ??? + } + + double simpleFunc(double val, int flag1, int flag2) + // simpleFunc: takes a value and calculates the expected ASIC output, given the two mode flags. + { - double simpleFunc(double val, int flag1, int flag2) - // simpleFunc: takes a value and calculates the expected ASIC output, given the two mode flags. - { if (flag1 > 0) return func1_muon(val, flag2); if (flag1 == -1) @@ -1915,14 +1904,14 @@ Small simple functions are easily inlined where the cost of a function call is s The (in)famous factorial: - constexpr int fac(int n) - { - constexpr int max_exp = 17; // constexpr enables this to be used in Expects - Expects(0<=n && n collect(istream& is) noexcept - { - vector res; - for(string s; is>>s; ) - res.push_back(s); - return res; - } + vector collect(istream& is) noexcept + { + vector res; + for(string s; is>>s; ) + res.push_back(s); + return res; + } If `collect()` runs out of memory, the program crashes. Unless the program is crafted to survive memory exhaustion, that may be just the right thing to do; @@ -2047,9 +2036,9 @@ Passing a shared smart pointer (e.g., `std::shared_ptr`) implies a run-time cost ##### Example - void f(int*); // accepts any int* - void g(unique_ptr); // can only accept ints for which you want to transfer ownership - void g(shared_ptr); // can only accept ints for which you are willing to share ownership + void f(int*); // accepts any int* + void g(unique_ptr); // can only accept ints for which you want to transfer ownership + void g(shared_ptr); // can only accept ints for which you are willing to share ownership ##### Note @@ -2069,8 +2058,8 @@ Flag smart pointer arguments. ##### Example - template - auto square(T t) { return t*t; } + template + auto square(T t) { return t*t; } ##### Note @@ -2100,11 +2089,11 @@ If you have multiple values to return, [use a tuple](#Rf-T-multi) or similar mul ##### Example - vector find_all(const vector&, int x); // return pointers to elements with the value x + vector find_all(const vector&, int x); // return pointers to elements with the value x ##### Example, bad - void find_all(const vector&, vector& out, int x); // place pointers to elements with value x in out + void find_all(const vector&, vector& out, int x); // place pointers to elements with value x in out ##### Exceptions @@ -2144,16 +2133,16 @@ Avoid "esoteric techniques" such as: Assuming that `Matrix` has move operations (possibly by keeping its elements in a `std::vector`. - Matrix operator+(const Matrix& a, const Matrix& b) - { - Matrix res; - // ... fill res with the sum ... - return res; - } + Matrix operator+(const Matrix& a, const Matrix& b) + { + Matrix res; + // ... fill res with the sum ... + return res; + } - Matrix x = m1+m2; // move constructor + Matrix x = m1+m2; // move constructor - y = m3+m3; // move assignment + y = m3+m3; // move assignment ##### Note @@ -2187,13 +2176,13 @@ Additionally, when debugging, `owner` and `not_null` can be instrumented Consider - int length(Record* p); + int length(Record* p); When I call `length(r)` should I test for `r==nullptr` first? Should the implementation of `length()` test for `p==nullptr`? - int length(not_null p); // it is the caller's job to make sure p!=nullptr + int length(not_null p); // it is the caller's job to make sure p!=nullptr - int length(Record* p); // the implementor of length() must assume that p==nullptr is possible + int length(Record* p); // the implementor of length() must assume that p==nullptr is possible ##### Note @@ -2211,7 +2200,6 @@ A `not_null` is assumed not to be the `nullptr`; a `T*` may be the `nullptr` * (Simple) ((Bounds)) Warn for any arithmetic operation on an expression of pointer type that results in a value of pointer type. - ### F.17: Use a `not_null` to indicate that "null" is not a valid value ##### Reason @@ -2220,14 +2208,14 @@ A `not_null` is assumed not to be the `nullptr`; a `T*` may be the `nullptr` ##### Example - not_null check(T* p) { if (p) return not_null{p}; throw Unexpected_nullptr{}; } + not_null check(T* p) { if (p) return not_null{p}; throw Unexpected_nullptr{}; } - void computer(not_null> p) - { - if (0> p) + { + if (0` is assumed not to be the `nullptr`; a `T*` may be the `nullptr` ##### Example - X* find(array_view r, const X& v); // find v in r + X* find(array_view r, const X& v); // find v in r - vector vec; - // ... - auto p = find({vec.begin(), vec.end()}, X{}); // find X{} in vec + vector vec; + // ... + auto p = find({vec.begin(), vec.end()}, X{}); // find X{} in vec ##### Note @@ -2282,13 +2270,13 @@ We must distinguish C-style strings from a pointer to a single character or an o Consider - int length(const char* p); + int length(const char* p); When I call `length(s)` should I test for `s==nullptr` first? Should the implementation of `length()` test for `p==nullptr`? - int length(zstring p); // the implementor of length() must assume that p==nullptr is possible + int length(zstring p); // the implementor of length() must assume that p==nullptr is possible - int length(not_null p); // it is the caller's job to make sure p!=nullptr + int length(not_null p); // it is the caller's job to make sure p!=nullptr ##### Note @@ -2304,9 +2292,9 @@ When I call `length(s)` should I test for `s==nullptr` first? Should the impleme ##### Example - void fct(const string& s); // OK: pass by const reference; always cheap + void fct(const string& s); // OK: pass by const reference; always cheap - void fct2(string s); // bad: potentially expensive + void fct2(string s); // bad: potentially expensive **Exception**: Sinks (that is, a function that eventually destroys an object or passes it along to another sink), may benefit ??? @@ -2330,11 +2318,11 @@ For small objects (up to two or three words) it is also faster than alternatives ##### Example - void fct(int x); // OK: Unbeatable + void fct(int x); // OK: Unbeatable - void fct2(const int& x); // bad: overhead on access in fct2() + void fct2(const int& x); // bad: overhead on access in fct2() - void fct(int& x); // OK, but means something else; use only for an "out parameter" + void fct(int& x); // OK, but means something else; use only for an "out parameter" ##### Enforcement @@ -2348,24 +2336,24 @@ For small objects (up to two or three words) it is also faster than alternatives ##### Example - void update(Record& r); // assume that update writes to r + void update(Record& r); // assume that update writes to r ##### Note A `T&` argument can pass information into a function as well as well as out of it. Thus `T&` could be and in-out-parameter. That can in itself be a problem and a source of errors: - void f(string& s) - { - s = "New York"; // non-obvious error - } + void f(string& s) + { + s = "New York"; // non-obvious error + } - string g() - { - string buffer = "................................."; - f(buffer); - // ... - } + string g() + { + string buffer = "................................."; + f(buffer); + // ... + } Here, the writer of `g()` is supplying a buffer for `f()` to fill, but `f()` simply replaces it (at a somewhat higher cost than a simple copy of the characters). @@ -2384,16 +2372,16 @@ If the writer of `g()` makes an assumption about the size of `buffer` a bad logi ##### Example - struct Package { - char header[16]; - char load[2024-16]; - }; + struct Package { + char header[16]; + char load[2024-16]; + }; - Package fill(); // Bad: large return value - void fill(Package&); // OK + Package fill(); // Bad: large return value + void fill(Package&); // OK - int val(); // OK - void val(int&); // Bad: Is val reading its argument + int val(); // OK + void val(int&); // Bad: Is val reading its argument ##### Enforcement @@ -2427,16 +2415,16 @@ If you have performance justification to optimize for rvalues, overload on `&&` ##### Example - void somefct(string&&); + void somefct(string&&); - void user() - { - string s = "this is going to be fun!"; - // ... - somefct(std::move(s)); // we don't need s any more, give it to somefct() - // - cout << s << '\n'; // Oops! What happens here? - } + void user() + { + string s = "this is going to be fun!"; + // ... + somefct(std::move(s)); // we don't need s any more, give it to somefct() + // + cout << s << '\n'; // Oops! What happens here? + } ##### Enforcement @@ -2451,16 +2439,16 @@ If you have performance justification to optimize for rvalues, overload on `&&` ##### Example - unique_ptr get_shape(istream& is) // assemble shape from input stream - { - auto kind = read_header(is); // read header and identify the next shape on input - switch (kind) { - case kCicle: - return make_unique(is); - case kTriangle: - return make_unique(is); - // ... - } + unique_ptr get_shape(istream& is) // assemble shape from input stream + { + auto kind = read_header(is); // read header and identify the next shape on input + switch (kind) { + case kCicle: + return make_unique(is); + case kTriangle: + return make_unique(is); + // ... + } ##### Note @@ -2478,15 +2466,15 @@ You need to pass a pointer rather than an object if what you are transferring is ##### Example - shared_ptr im { read_image(somewhere) }; + shared_ptr im { read_image(somewhere) }; - std::thread t0 {shade, args0, top_left, im}; - std::thread t1 {shade, args1, top_right, im}; - std::thread t2 {shade, args2, bottom_left, im}; - std::thread t3 {shade, args3, bottom_right, im}; + std::thread t0 {shade, args0, top_left, im}; + std::thread t1 {shade, args1, top_right, im}; + std::thread t2 {shade, args2, bottom_left, im}; + std::thread t3 {shade, args3, bottom_right, im}; - // detach treads - // last thread to finish deletes the image + // detach treads + // last thread to finish deletes the image ##### Note @@ -2507,12 +2495,12 @@ Prefer a `unique_ptr` over a `shared_ptr` if there is never more than one owner ##### Example - void incr(int&); - int incr(int); + void incr(int&); + int incr(int); - int i = 0; - incr(i); - i = incr(i); + int i = 0; + incr(i); + i = incr(i); ##### Enforcement @@ -2542,16 +2530,16 @@ In fact, C++98's standard library already used this convenient feature, because For example, given a `set myset`, consider: // C++98 - result = myset.insert( "Hello" ); - if (result.second) do_something_with( result.first ); // workaround + result = myset.insert( "Hello" ); + if (result.second) do_something_with( result.first ); // workaround With C++11 we can write this, putting the results directly in existing local variables: - Sometype iter; // default initialize if we haven't already - Someothertype success; // used these variables for some other purpose + Sometype iter; // default initialize if we haven't already + Someothertype success; // used these variables for some other purpose - tie( iter, success ) = myset.insert( "Hello" ); // normal return value - if (success) do_something_with( iter ); + tie( iter, success ) = myset.insert( "Hello" ); // normal return value + if (success) do_something_with( iter ); **Exception**: For types like `string` and `vector` that carry additional capacity, it can sometimes be useful to treat it as in/out instead by using the "caller-allocated out" pattern, which is to pass an output-only object by reference to non-`const` so that when the callee writes to it the object can reuse any capacity or other resources that it already contains. This technique can dramatically reduce the number of allocations in a loop that repeatedly calls other functions to get string values, by using a single string object for the entire loop. @@ -2595,21 +2583,21 @@ Positions can also be transferred by iterators, indices, and references. ##### Example, bad - int* f() - { - int x = 7; - // ... - return &x; // Bad: returns pointer to object that is about to be destroyed - } + int* f() + { + int x = 7; + // ... + return &x; // Bad: returns pointer to object that is about to be destroyed + } This applies to references as well: - int& f() - { - int x = 7; - // ... - return x; // Bad: returns reference to object that is about to be destroyed - } + int& f() + { + int x = 7; + // ... + return x; // Bad: returns reference to object that is about to be destroyed + } **See also**: [discussion of dangling pointer prevention](#???). @@ -2630,32 +2618,32 @@ A slightly different variant of the problem is placing pointers in a container t After the return from a function its local objects no longer exist: - int* f() - { - int fx = 9; - return &fx; // BAD - } + int* f() + { + int fx = 9; + return &fx; // BAD + } - void g(int* p) // looks innocent enough - { - int gx; - cout << "*p == " << *p << '\n'; - *p = 999; - cout << "gx == " << gx << '\n'; - } + void g(int* p) // looks innocent enough + { + int gx; + cout << "*p == " << *p << '\n'; + *p = 999; + cout << "gx == " << gx << '\n'; + } - void h() - { - int* p = f(); - int z = *p; // read from abandoned stack frame (bad) - g(p); // pass pointer to abandoned stack frame to function (bad) + void h() + { + int* p = f(); + int z = *p; // read from abandoned stack frame (bad) + g(p); // pass pointer to abandoned stack frame to function (bad) - } + } Here on one popular implementation I got the output - *p == 999 - gx == 999 + *p == 999 + gx == 999 I expected that because the call of `g()` reuses the stack space abandoned by the call of `f()` so `*p` refers to the space now occupied by `gx`. @@ -2679,25 +2667,25 @@ All `static` variables are (as their name indicates) statically allocated, so th Not all examples of leaking a pointer to a local variable are that obvious: - int* glob; // global variables are bad in so many ways + int* glob; // global variables are bad in so many ways - template - void steal(T x) - { - glob = x(); // BAD - } + template + void steal(T x) + { + glob = x(); // BAD + } - void f() - { - int i = 99; - steal([&] { return &i; }); - } + void f() + { + int i = 99; + steal([&] { return &i; }); + } - int main() - { - f(); - cout << *glob << '\n'; - } + int main() + { + f(); + cout << *glob << '\n'; + } Here I managed to read the location abandoned by the call of `f`. The pointer stored in `glob` could be used much later and cause trouble in unpredictable ways. @@ -2730,7 +2718,7 @@ Preventable through static analysis. ##### Example - ??? + ??? ##### Enforcement @@ -2748,21 +2736,21 @@ For passthrough functions that pass in parameters (by ordinary reference or by p If `F` returns by value, this function returns a reference to a temporary. - template - auto&& wrapper(F f) { - log_call(typeid(f)); // or whatever instrumentation - return f(); - } + template + auto&& wrapper(F f) { + log_call(typeid(f)); // or whatever instrumentation + return f(); + } ##### Example, good Better: - template - auto wrapper(F f) { - log_call(typeid(f)); // or whatever instrumentation - return f(); - } + template + auto wrapper(F f) { + log_call(typeid(f)); // or whatever instrumentation + return f(); + } **Exception**: `std::move` and `std::forward` do return `&&`, but they are just casts -- used by convention only in expression contexts where a reference to a temporary object is passed along within the same expression before the temporary is destroyed. We don't know of any other good examples of returning `&&`. @@ -2778,23 +2766,23 @@ Flag any use of `&&` as a return type, except in `std::move` and `std::forward`. ##### Example - // writing a function that should only take an int or a string -- overloading is natural - void f(int); - void f(const string&); + // writing a function that should only take an int or a string -- overloading is natural + void f(int); + void f(const string&); - // writing a function object that needs to capture local state and appear - // at statement or expression scope -- a lambda is natural - vector v = lots_of_work(); - for(int tasknum = 0; tasknum < max; ++tasknum) { - pool.run([=, &v]{ - /* - ... - ... process 1/max-th of v, the tasknum-th chunk - ... - */ - }); - } - pool.join(); + // writing a function object that needs to capture local state and appear + // at statement or expression scope -- a lambda is natural + vector v = lots_of_work(); + for(int tasknum = 0; tasknum < max; ++tasknum) { + pool.run([=, &v]{ + /* + ... + ... process 1/max-th of v, the tasknum-th chunk + ... + */ + }); + } + pool.join(); **Exception**: Generic lambdas offer a concise way to write function templates and so can be useful even when a normal function template would do equally well with a little more syntax. This advantage will probably disappear in the future once all functions gain the ability to have Concept parameters. @@ -2803,6 +2791,7 @@ Flag any use of `&&` as a return type, except in `std::move` and `std::forward`. * Warn on use of a named non-generic lambda (e.g., `auto x = [](int i){ /*...*/; };`) that captures nothing and appears at global scope. Write an ordinary function instead. ### F.51: Prefer overloading over default arguments for virtual functions + ??? possibly other situations? ##### Reason @@ -2811,21 +2800,21 @@ Flag any use of `&&` as a return type, except in `std::move` and `std::forward`. ##### Example, bad - class base { - public: - virtual int multiply(int value, int factor = 2) = 0; - }; + class base { + public: + virtual int multiply(int value, int factor = 2) = 0; + }; - class derived : public base { - public: - int multiply(int value, int factor = 10) override; - }; + class derived : public base { + public: + int multiply(int value, int factor = 10) override; + }; - derived d; - base& b = d; + derived d; + base& b = d; - b.multiply(10); // these two calls will call the same function but - d.multiply(10); // with different arguments and so different results + b.multiply(10); // these two calls will call the same function but + d.multiply(10); // with different arguments and so different results ##### Enforcement @@ -2841,12 +2830,12 @@ Flag all uses of default arguments in virtual functions. This is a simple three-stage parallel pipeline. Each `stage` object encapsulates a worker thread and a queue, has a `process` function to enqueue work, and in its destructor automatically blocks waiting for the queue to empty before ending the thread. - void send_packets( buffers& bufs ) { - stage encryptor ([] (buffer& b){ encrypt(b); }); - stage compressor ([&](buffer& b){ compress(b); encryptor.process(b); }); - stage decorator ([&](buffer& b){ decorate(b); compressor.process(b); }); - for (auto& b : bufs) { decorator.process(b); } - } // automatically blocks waiting for pipeline to finish + void send_packets( buffers& bufs ) { + stage encryptor ([] (buffer& b){ encrypt(b); }); + stage compressor ([&](buffer& b){ compress(b); encryptor.process(b); }); + stage decorator ([&](buffer& b){ decorate(b); compressor.process(b); }); + for (auto& b : bufs) { decorator.process(b); } + } // automatically blocks waiting for pipeline to finish ##### Enforcement @@ -2860,12 +2849,12 @@ This is a simple three-stage parallel pipeline. Each `stage` object encapsulates ##### Example - { - // ... + { + // ... - // a, b, c are local variables - background_thread.queue_work([=]{ process(a, b, c); }); // want copies of a, b, and c - } + // a, b, c are local variables + background_thread.queue_work([=]{ process(a, b, c); }); // want copies of a, b, and c + } ##### Enforcement @@ -2903,8 +2892,8 @@ Subsections: ##### Example - void draw(int x, int y, int x2, int y2); // BAD: unnecessary implicit relationships - void draw(Point from, Point to); // better + void draw(int x, int y, int x2, int y2); // BAD: unnecessary implicit relationships + void draw(Point from, Point to); // better ##### Note @@ -2930,22 +2919,22 @@ An invariant is a logical condition for the members of an object that a construc ##### Example - struct Pair { // the members can vary independently - string name; - int volume; - }; + struct Pair { // the members can vary independently + string name; + int volume; + }; but - class Date { - private: - int y; - Month m; - char d; // day - public: - Date(int yy, Month mm, char dd); // validate that {yy, mm, dd} is a valid date and initialize - // ... - }; + class Date { + private: + int y; + Month m; + char d; // day + public: + Date(int yy, Month mm, char dd); // validate that {yy, mm, dd} is a valid date and initialize + // ... + }; ##### Enforcement @@ -2959,16 +2948,16 @@ Look for `struct`s with all data private and `class`es with public members. ##### Example - class Date { - // ... some representation ... - public: - Date(); - Date(int yy, Month mm, char dd); // validate that {yy, mm, dd} is a valid date and initialize + class Date { + // ... some representation ... + public: + Date(); + Date(int yy, Month mm, char dd); // validate that {yy, mm, dd} is a valid date and initialize - int day() const; - Month month() const; - // ... - }; + int day() const; + Month month() const; + // ... + }; For example, we can now change the representation of a `Date` without affecting its users (recompilation is likely, though). @@ -2993,13 +2982,13 @@ Ideally, and typically, an interface is far more stable than its implementation( ##### Example - class Date { - // ... relatively small interface ... - }; + class Date { + // ... relatively small interface ... + }; - // helper functions: - Date next_weekday(Date); - bool operator==(Date, Date); + // helper functions: + Date next_weekday(Date); + bool operator==(Date, Date); The "helper functions" have no need for direct access to the representation of a `Date`. @@ -3022,16 +3011,16 @@ Placing them in the same namespace as the class makes their relationship to the ##### Example - namespace Chrono { // here we keep time-related services + namespace Chrono { // here we keep time-related services - class Time { /* ... */ }; - class Date { /* ... */ }; + class Time { /* ... */ }; + class Date { /* ... */ }; - // helper functions: - bool operator==(Date, Date); - Date next_weekday(Date); - // ... - } + // helper functions: + bool operator==(Date, Date); + Date next_weekday(Date); + // ... + } ##### Enforcement @@ -3045,7 +3034,7 @@ Placing them in the same namespace as the class makes their relationship to the ##### Example - int Date::day() const { return d; } + int Date::day() const { return d; } ##### Note @@ -3070,7 +3059,6 @@ Concrete type rule summary: * [C.10: Prefer a concrete type over more complicated classes](#Rc-concrete) * [C.11: Make concrete types regular](#Rc-regular) - ### C.10 Prefer a concrete type over more complicated classes ##### Reason @@ -3081,28 +3069,28 @@ You need a reason (use cases) for using a hierarchy. ##### Example - class Point1 { - int x, y; - // ... operations ... - // .. no virtual functions ... - }; + class Point1 { + int x, y; + // ... operations ... + // .. no virtual functions ... + }; - class Point2 { - int x, y; - // ... operations, some virtual ... - virtual ~Point2(); - }; + class Point2 { + int x, y; + // ... operations, some virtual ... + virtual ~Point2(); + }; - void use() - { - Point1 p11 {1, 2}; // make an object on the stack - Point1 p12 {p11}; // a copy + void use() + { + Point1 p11 {1, 2}; // make an object on the stack + Point1 p12 {p11}; // a copy - auto p21 = make_unique(1, 2); // make an object on the free store - auto p22 = p21.clone(); // make a copy + auto p21 = make_unique(1, 2); // make an object on the free store + auto p22 = p21.clone(); // make a copy - // ... - } + // ... + } If a class can be part of a hierarchy, we (in real code if not necessarily in small examples) must manipulate its objects through pointers or references. That implies more memory overhead, more allocations and deallocations, and more run-time overhead to perform the resulting indirections. @@ -3130,18 +3118,18 @@ This is done where dynamic allocation is prohibited (e.g. hard real-time) and to ##### Example - struct Bundle { - string name; - vector vr; - }; + struct Bundle { + string name; + vector vr; + }; - bool operator==(const Bundle& a, const Bundle& b) { return a.name==b.name && a.vr==b.vr; } + bool operator==(const Bundle& a, const Bundle& b) { return a.name==b.name && a.vr==b.vr; } - Bundle b1 { "my bundle", {r1, r2, r3}}; - Bundle b2 = b1; - if (!(b1==b2)) error("impossible!"); - b2.name = "the other bundle"; - if (b1==b2) error("No!"); + Bundle b1 { "my bundle", {r1, r2, r3}}; + Bundle b2 = b1; + if (!(b1==b2)) error("impossible!"); + b2.name = "the other bundle"; + if (b1==b2) error("No!"); In particular, if a concrete type has an assignment also give it an equals operator so that `a=b` implies `a==b`. @@ -3225,13 +3213,11 @@ Other default operations rules: * [C.88: Make `<` symmetric with respect of operand types and `noexcept`](#Rc-lt) * [C.89: Make a `hash` `noexcept`](#Rc-hash) - ## C.defop: Default Operations By default, the language supply the default operations with their default semantics. However, a programmer can disable or replace these defaults. - ### C.20: If you can avoid defining default operations, do ##### Reason @@ -3240,16 +3226,16 @@ However, a programmer can disable or replace these defaults. ##### Example - struct Named_map { - public: - // ... no default operations declared ... - private: - string name; - map rep; - }; + struct Named_map { + public: + // ... no default operations declared ... + private: + string name; + map rep; + }; - Named_map nm; // default construct - Named_map nm2 {nm}; // copy construct + Named_map nm; // default construct + Named_map nm2 {nm}; // copy construct Since `std::map` and `string` have all the special functions, no further work is needed. @@ -3270,23 +3256,23 @@ This is known as "the rule of zero". ##### Example, bad - struct M2 { // bad: incomplete set of default operations - public: - // ... - // ... no copy or move operations ... - ~M2() { delete[] rep; } - private: - pair* rep; // zero-terminated set of pairs - }; + struct M2 { // bad: incomplete set of default operations + public: + // ... + // ... no copy or move operations ... + ~M2() { delete[] rep; } + private: + pair* rep; // zero-terminated set of pairs + }; - void use() - { - M2 x; - M2 y; - // ... - x = y; // the default assignment - // ... - } + void use() + { + M2 x; + M2 y; + // ... + x = y; // the default assignment + // ... + } Given that "special attention" was needed for the destructor (here, to deallocate), the likelihood that copy and move assignment (both will implicitly destroy an object) are correct is low (here, we would get double deletion). @@ -3320,16 +3306,16 @@ Users will be surprised if copy/move construction and copy/move assignment do lo ##### Example, bad - class Silly { // BAD: Inconsistent copy operations - class Impl { - // ... - }; + class Silly { // BAD: Inconsistent copy operations + class Impl { + // ... + }; shared_ptr p; - public: - Silly(const Silly& a) : p{a.p} { *p = *a.p; } // deep copy - Silly& operator=(const Silly& a) { p = a.p; } // shallow copy - // ... - }; + public: + Silly(const Silly& a) : p{a.p} { *p = *a.p; } // deep copy + Silly& operator=(const Silly& a) { p = a.p; } // shallow copy + // ... + }; These operations disagree about copy semantics. This will lead to confusion and bugs. @@ -3357,26 +3343,26 @@ Only define a non-default destructor if a class needs to execute code that is no ##### Example - template - struct final_action { // slightly simplified - A act; - final_action(F a) :act{a} {} - ~final_action() { act(); } - }; + template + struct final_action { // slightly simplified + A act; + final_action(F a) :act{a} {} + ~final_action() { act(); } + }; - template - final_action finally(A act) // deduce action type - { - return final_action{a}; - } + template + final_action finally(A act) // deduce action type + { + return final_action{a}; + } - void test() - { - auto act = finally([]{ cout<<"Exit test\n"; }); // establish exit action - // ... - if (something) return; // act done here - // ... - } // act done here + void test() + { + auto act = finally([]{ cout<<"Exit test\n"; }); // establish exit action + // ... + if (something) return; // act done here + // ... + } // act done here The whole purpose of `final_action` is to get a piece of code (usually a lambda) executed upon destruction. @@ -3389,15 +3375,15 @@ There are two general categories of classes that need a user-defined destructor: ##### Example, bad - class Foo { // bad; use the default destructor - public: - // ... - ~Foo() { s=""; i=0; vi.clear(); } // clean up - private: - string s; - int i; - vector vi; - }; + class Foo { // bad; use the default destructor + public: + // ... + ~Foo() { s=""; i=0; vi.clear(); } // clean up + private: + string s; + int i; + vector vi; + }; The default destructor does it better, more efficiently, and can't get it wrong. @@ -3421,19 +3407,19 @@ For resources represented as classes with a complete set of default operations, ##### Example - class X { - ifstream f; // may own a file - // ... no default operations defined or =deleted ... - }; + class X { + ifstream f; // may own a file + // ... no default operations defined or =deleted ... + }; `X`'s `ifstream` implicitly closes any file it may have open upon destruction of its `X`. ##### Example, bad - class X2 { // bad - FILE* f; // may own a file - // ... no default operations defined or =deleted ... - }; + class X2 { // bad + FILE* f; // may own a file + // ... no default operations defined or =deleted ... + }; `X2` may leak a file handle. @@ -3453,9 +3439,9 @@ A class can hold pointers and references to objects that it does not own. Obviously, such objects should not be `delete`d by the class's destructor. For example: - Preprocessor pp { /* ... */ }; - Parser p { pp, /* ... */ }; - Type_checker tc { p, /* ... */ }; + Preprocessor pp { /* ... */ }; + Parser p { pp, /* ... */ }; + Type_checker tc { p, /* ... */ }; Here `p` refers to `pp` but does not own it. @@ -3474,7 +3460,7 @@ Here `p` refers to `pp` but does not own it. ##### Example - ??? + ??? ##### Note @@ -3497,54 +3483,53 @@ A pointer member may represent a resource. [A `T*` should not do so](#Rr-ptr), but in older code, that's common. Consider a `T*` a possible owner and therefore suspect. - template - class Smart_ptr { - T* p; // BAD: vague about ownership of *p - // ... - public: - // ... no user-defined default operations ... - }; + template + class Smart_ptr { + T* p; // BAD: vague about ownership of *p + // ... + public: + // ... no user-defined default operations ... + }; - void use(Smart_ptr p1) - { - auto p2 = p1; // error: p2.p leaked (if not nullptr and not owned by some other code) - } + void use(Smart_ptr p1) + { + auto p2 = p1; // error: p2.p leaked (if not nullptr and not owned by some other code) + } Note that if you define a destructor, you must define or delete [all default operations](#Rc-five): - template - class Smart_ptr2 { - T* p; // BAD: vague about ownership of *p - // ... - public: - // ... no user-defined copy operations ... - ~Smart_ptr2() { delete p; } // p is an owner! - }; + template + class Smart_ptr2 { + T* p; // BAD: vague about ownership of *p + // ... + public: + // ... no user-defined copy operations ... + ~Smart_ptr2() { delete p; } // p is an owner! + }; - void use(Smart_ptr p1) - { - auto p2 = p1; // error: double deletion - } + void use(Smart_ptr p1) + { + auto p2 = p1; // error: double deletion + } The default copy operation will just copy the `p1.p` into `p2.p` leading to a double destruction of `p1.p`. Be explicit about ownership: - template - class Smart_ptr3 { - owner* p; // OK: explicit about ownership of *p - // ... - public: - // ... - // ... copy and move operations ... - ~Smart_ptr3() { delete p; } - }; + template + class Smart_ptr3 { + owner* p; // OK: explicit about ownership of *p + // ... + public: + // ... + // ... copy and move operations ... + ~Smart_ptr3() { delete p; } + }; - void use(Smart_ptr3 p1) - { - auto p2 = p1; // error: double deletion - } + void use(Smart_ptr3 p1) + { + auto p2 = p1; // error: double deletion + } - - ##### Note +##### Note Often the simplest way to get a destructor is to replace the pointer with a smart pointer (e.g., `std::unique_ptr`) and let the compiler arrange for proper destruction to be done implicitly. @@ -3570,34 +3555,34 @@ Also, copying may lead to slicing. ##### Example, bad - class Handle { // Very suspect - Shape& s; // use reference rather than pointer to prevent rebinding - // BAD: vague about ownership of *p - // ... - public: - Handle(Shape& ss) : s{ss} { /* ... */ } - // ... - }; + class Handle { // Very suspect + Shape& s; // use reference rather than pointer to prevent rebinding + // BAD: vague about ownership of *p + // ... + public: + Handle(Shape& ss) : s{ss} { /* ... */ } + // ... + }; The problem of whether `Handle` is responsible for the destruction of its `Shape` is the same as for [the pointer case](#Rc-dtor-ptr): If the `Handle` owns the object referred to by `s` it must have a destructor. ##### Example - class Handle { // OK - owner s; // use reference rather than pointer to prevent rebinding - // ... - public: - Handle(Shape& ss) : s{ss} { /* ... */ } - ~Handle() { delete &s; } - // ... - }; + class Handle { // OK + owner s; // use reference rather than pointer to prevent rebinding + // ... + public: + Handle(Shape& ss) : s{ss} { /* ... */ } + ~Handle() { delete &s; } + // ... + }; Independently of whether `Handle` owns its `Shape`, we must consider the default copy operations suspect: - Handle x {*new Circle{p1, 17}}; // the Handle had better own the Circle or we have a leak - Handle y {*new Triangle{p1, p2, p3}}; - x = y; // the default assignment will try *x.s=*y.s + Handle x {*new Circle{p1, 17}}; // the Handle had better own the Circle or we have a leak + Handle y {*new Triangle{p1, p2, p3}}; + x = y; // the default assignment will try *x.s=*y.s That `x=y` is highly suspect. Assigning a `Triangle` to a `Circle`? @@ -3625,21 +3610,21 @@ In general, the writer of a base class does not know the appropriate action to b ##### Example, bad - struct Base { // BAD: no virtual destructor - virtual f(); - }; + struct Base { // BAD: no virtual destructor + virtual f(); + }; - struct D : Base { - string s {"a resource needing cleanup"}; - ~D() { /* ... do some cleanup ... */ } - // ... - }; + struct D : Base { + string s {"a resource needing cleanup"}; + ~D() { /* ... do some cleanup ... */ } + // ... + }; void use() - { - unique_ptr p = make_unique(); + { + unique_ptr p = make_unique(); // ... - } // p's destruction calls ~Base(), not ~D(), which leaks D::s and possibly more + } // p's destruction calls ~Base(), not ~D(), which leaks D::s and possibly more ##### Note @@ -3650,16 +3635,16 @@ Someone using such an interface is likely to also destroy using that interface. A destructor must be `public` or it will prevent stack allocation and normal heap allocation via smart pointer (or in legacy code explicit `delete`): - class X { - ~X(); // private destructor - // ... - }; + class X { + ~X(); // private destructor + // ... + }; - void use() - { - X a; // error: cannot destroy - auto p = make_unique(); // error: cannot destroy - } + void use() + { + X a; // error: cannot destroy + auto p = make_unique(); // error: cannot destroy + } ##### Enforcement @@ -3674,18 +3659,18 @@ The standard library requires that all classes it deals with have destructors th ##### Example - class X { - public: - ~X() noexcept; - // ... - }; + class X { + public: + ~X() noexcept; + // ... + }; - X::~X() noexcept - { - // ... - if (cannot_release_a_resource) terminate(); - // ... - } + X::~X() noexcept + { + // ... + if (cannot_release_a_resource) terminate(); + // ... + } ##### Note @@ -3741,17 +3726,17 @@ A constructor defined how an object is initialized (constructed). ##### Example - class Date { // a Date represents a valid date - // in the January 1, 1900 to December 31, 2100 range - Date(int dd, int mm, int yy) - :d{dd}, m{mm}, y{yy} - { - if (!is_valid(d, m, y)) throw Bad_date{}; // enforce invariant - } - // ... - private: - int d, m, y; - }; + class Date { // a Date represents a valid date + // in the January 1, 1900 to December 31, 2100 range + Date(int dd, int mm, int yy) + :d{dd}, m{mm}, y{yy} + { + if (!is_valid(d, m, y)) throw Bad_date{}; // enforce invariant + } + // ... + private: + int d, m, y; + }; It is often a good idea to express the invariant as an `Ensure` on the constructor. @@ -3759,28 +3744,28 @@ It is often a good idea to express the invariant as an `Ensure` on the construct A constructor can be used for convenience even if a class does not have an invariant. For example: - struct Rec { - string s; - int i {0}; - Rec(const string& ss) : s{ss} {} - Rec(int ii) :i{ii} {} - }; + struct Rec { + string s; + int i {0}; + Rec(const string& ss) : s{ss} {} + Rec(int ii) :i{ii} {} + }; - Rec r1 {7}; - Rec r2 {"Foo bar"}; + Rec r1 {7}; + Rec r2 {"Foo bar"}; ##### Note The C++11 initializer list rules eliminates the need for many constructors. For example: - struct Rec2{ - string s; - int i; - Rec2(const string& ss, int ii = 0) :s{ss}, i{ii} {} // redundant - }; + struct Rec2{ + string s; + int i; + Rec2(const string& ss, int ii = 0) :s{ss}, i{ii} {} // redundant + }; - Rec r1 {"Foo", 7}; - Rec r2 {"Bar"}; + Rec r1 {"Foo", 7}; + Rec r2 {"Bar"}; The `Rec2` constructor is redundant. Also, the default for `int` would be better done as a [member initializer](#Rc-in-class-initializer). @@ -3799,24 +3784,24 @@ Also, the default for `int` would be better done as a [member initializer](#Rc-i ##### Example, bad - class X1 { - FILE* f; // call init() before any other function - // ... - public: - X1() {} - void init(); // initialize f - void read(); // read from f - // ... - }; + class X1 { + FILE* f; // call init() before any other function + // ... + public: + X1() {} + void init(); // initialize f + void read(); // read from f + // ... + }; - void f() - { - X1 file; - file.read(); // crash or bad read! - // ... - file.init(); // too late - // ... - } + void f() + { + X1 file; + file.read(); // crash or bad read! + // ... + file.init(); // too late + // ... + } Compilers do not read comments. @@ -3835,61 +3820,61 @@ The idiom of having constructors acquire resources and destructors release them ##### Example - class X2 { - FILE* f; // call init() before any other function - // ... - public: - X2(const string& name) - :f{fopen(name.c_str(), "r")} - { - if (f==nullptr) throw runtime_error{"could not open" + name}; - // ... - } + class X2 { + FILE* f; // call init() before any other function + // ... + public: + X2(const string& name) + :f{fopen(name.c_str(), "r")} + { + if (f==nullptr) throw runtime_error{"could not open" + name}; + // ... + } - void read(); // read from f - // ... - }; + void read(); // read from f + // ... + }; - void f() - { - X2 file {"Zeno"}; // throws if file isn't open - file.read(); // fine - // ... - } + void f() + { + X2 file {"Zeno"}; // throws if file isn't open + file.read(); // fine + // ... + } ##### Example, bad - class X3 { // bad: the constructor leaves a non-valid object behind - FILE* f; // call init() before any other function - bool valid; - // ... - public: - X3(const string& name) - :f{fopen(name.c_str(), "r")}, valid{false} - { - if (f) valid=true; - // ... - } + class X3 { // bad: the constructor leaves a non-valid object behind + FILE* f; // call init() before any other function + bool valid; + // ... + public: + X3(const string& name) + :f{fopen(name.c_str(), "r")}, valid{false} + { + if (f) valid=true; + // ... + } - void is_valid() { return valid; } - void read(); // read from f - // ... - }; + void is_valid() { return valid; } + void read(); // read from f + // ... + }; - void f() - { - X3 file {"Heraclides"}; - file.read(); // crash or bad read! - // ... - if (is_valid()) { - file.read(); - // ... - } - else { - // ... handle error ... - } - // ... - } + void f() + { + X3 file {"Heraclides"}; + file.read(); // crash or bad read! + // ... + if (is_valid()) { + file.read(); + // ... + } + else { + // ... handle error ... + } + // ... + } ##### Note @@ -3913,14 +3898,14 @@ e.g. `T a[10]` and `std::vector v(10)` default initializes their elements. ##### Example - class Date { - public: - Date(); - // ... - }; + class Date { + public: + Date(); + // ... + }; - vector vd1(1000); // default Date needed here - vector vd2(1000, Date{Month::october, 7, 1885}); // alternative + vector vd1(1000); // default Date needed here + vector vd2(1000, Date{Month::october, 7, 1885}); // alternative There is no "natural" default date (the big bang is too far back in time to be useful for most people), so this example is non-trivial. `{0, 0, 0}` is not a valid date in most calendar systems, so choosing that would be introducing something like floating-point's NaN. @@ -3930,7 +3915,6 @@ However, most realistic `Date` classes has a "first date" (e.g. January 1, 1970 * Flag classes without a default constructor - ### C.44: Prefer default constructors to be simple and non-throwing ##### Reason @@ -3939,17 +3923,17 @@ However, most realistic `Date` classes has a "first date" (e.g. January 1, 1970 ##### Example, problematic - template - class Vector0 { // elem points to space-elem element allocated using new - public: - Vector0() :Vector0{0} {} - Vector0(int n) :elem{new T[n]}, space{elem+n}, last{elem} {} - // ... - private: - own elem; - T* space; - T* last; - }; + template + class Vector0 { // elem points to space-elem element allocated using new + public: + Vector0() :Vector0{0} {} + Vector0(int n) :elem{new T[n]}, space{elem+n}, last{elem} {} + // ... + private: + own elem; + T* space; + T* last; + }; This is nice and general, but setting a `Vector0` to empty after an error involves an allocation, which may fail. Also, having a default `Vector` represented as `{new T[0], 0, 0}` seems wasteful. @@ -3957,17 +3941,17 @@ For example, `Vector0 v(100)` costs 100 allocations. ##### Example - template - class Vector1 { // elem is nullptr or elem points to space-elem element allocated using new - public: - Vector1() noexcept {} // sets the representation to {nullptr, nullptr, nullptr}; doesn't throw - Vector1(int n) :elem{new T[n]}, space{elem+n}, last{elem} {} - // ... - private: - own elem = nullptr; - T* space = nullptr; - T* last = nullptr; - }; + template + class Vector1 { // elem is nullptr or elem points to space-elem element allocated using new + public: + Vector1() noexcept {} // sets the representation to {nullptr, nullptr, nullptr}; doesn't throw + Vector1(int n) :elem{new T[n]}, space{elem+n}, last{elem} {} + // ... + private: + own elem = nullptr; + T* space = nullptr; + T* last = nullptr; + }; Using `{nullptr, nullptr, nullptr}` makes `Vector1{}` cheap, but a special case and implies run-time checks. Setting a `Vector1` to empty after detecting an error is trivial. @@ -3989,7 +3973,7 @@ Setting a `Vector1` to empty after detecting an error is trivial. int i; public: X1() :s{"default"}, i{1} { } - // ... + // ... }; ##### Example @@ -3999,7 +3983,7 @@ Setting a `Vector1` to empty after detecting an error is trivial. int i = 1; public: // use compiler-generated default constructor - // ... + // ... }; ##### Enforcement @@ -4014,27 +3998,27 @@ Setting a `Vector1` to empty after detecting an error is trivial. ##### Example, bad - class String { - // ... - public: - String(int); // BAD - // ... - }; + class String { + // ... + public: + String(int); // BAD + // ... + }; - String s = 10; // surprise: string of size 10 + String s = 10; // surprise: string of size 10 ##### Exception If you really want an implicit conversion from the constructor argument type to the class type, don't use `explicit`: - class Complex { - // ... - public: - Complex(double d); // OK: we want a conversion from d to {d, 0} - // ... - }; + class Complex { + // ... + public: + Complex(double d); // OK: we want a conversion from d to {d, 0} + // ... + }; - Complex z = 10.7; // unsurprising conversion + Complex z = 10.7; // unsurprising conversion **See also**: [Discussion of implicit conversions](#Ro-conversion). @@ -4050,15 +4034,15 @@ If you really want an implicit conversion from the constructor argument type to ##### Example, bad - class Foo { - int m1; - int m2; - public: - Foo(int x) :m2{x}, m1{++x} { } // BAD: misleading initializer order - // ... - }; + class Foo { + int m1; + int m2; + public: + Foo(int x) :m2{x}, m1{++x} { } // BAD: misleading initializer order + // ... + }; - Foo x(1); // surprise: x.m1==x.m2==2 + Foo x(1); // surprise: x.m1==x.m2==2 ##### Enforcement @@ -4074,41 +4058,41 @@ If you really want an implicit conversion from the constructor argument type to ##### Example, bad - class X { // BAD - int i; - string s; - int j; - public: - X() :i{666}, s{"qqq"} { } // j is uninitialized - X(int ii) :i{ii} {} // s is "" and j is uninitialized - // ... - }; + class X { // BAD + int i; + string s; + int j; + public: + X() :i{666}, s{"qqq"} { } // j is uninitialized + X(int ii) :i{ii} {} // s is "" and j is uninitialized + // ... + }; How would a maintainer know whether `j` was deliberately uninitialized (probably a poor idea anyway) and whether it was intentional to give `s` the default value `""` in one case and `qqq` in another (almost certainly a bug)? The problem with `j` (forgetting to initialize a member) often happens when a new member is added to an existing class. ##### Example - class X2 { - int i {666}; - string s {"qqq"}; - int j {0}; - public: - X2() = default; // all members are initialized to their defaults - X2(int ii) :i{ii} {} // s and j initialized to their defaults - // ... - }; + class X2 { + int i {666}; + string s {"qqq"}; + int j {0}; + public: + X2() = default; // all members are initialized to their defaults + X2(int ii) :i{ii} {} // s and j initialized to their defaults + // ... + }; **Alternative**: We can get part of the benefits from default arguments to constructors, and that is not uncommon in older code. However, that is less explicit, causes more arguments to be passed, and is repetitive when there is more than one constructor: - class X3 { // BAD: inexplicit, argument passing overhead - int i; - string s; - int j; - public: - X3(int ii = 666, const string& ss = "qqq", int jj = 0) - :i{ii}, s{ss}, j{jj} { } // all members are initialized to their defaults - // ... - }; + class X3 { // BAD: inexplicit, argument passing overhead + int i; + string s; + int j; + public: + X3(int ii = 666, const string& ss = "qqq", int jj = 0) + :i{ii}, s{ss}, j{jj} { } // all members are initialized to their defaults + // ... + }; ##### Enforcement @@ -4123,28 +4107,28 @@ How would a maintainer know whether `j` was deliberately uninitialized (probably ##### Example, good - class A { // Good - string s1; - public: - A() : s1{"Hello, "} { } // GOOD: directly construct - // ... - }; + class A { // Good + string s1; + public: + A() : s1{"Hello, "} { } // GOOD: directly construct + // ... + }; ##### Example, bad - class B { // BAD + class B { // BAD string s1; - public: + public: B() { s1 = "Hello, "; } // BAD: default constructor followed by assignment - // ... - }; + // ... + }; - class C { // UGLY, aka very bad - int* p; - public: - C() { cout << *p; p = new int{10}; } // accidental use before initialized - // ... - }; + class C { // UGLY, aka very bad + int* p; + public: + C() { cout << *p; p = new int{10}; } // accidental use before initialized + // ... + }; ### C.50: Use a factory function if you need "virtual behavior" during initialization @@ -4155,19 +4139,19 @@ How would a maintainer know whether `j` was deliberately uninitialized (probably ##### Example, bad - class B { - public: - B() - { - // ... - f(); // BAD: virtual call in constructor - //... - } + class B { + public: + B() + { + // ... + f(); // BAD: virtual call in constructor + //... + } - virtual void f() = 0; + virtual void f() = 0; - // ... - }; + // ... + }; ##### Example @@ -4215,38 +4199,38 @@ Conventional factory functions allocate on the free store, rather than on the st ##### Example, bad - class Date { // BAD: repetitive - int d; - Month m; - int y; - public: - Date(int ii, Month mm, year yy) - :i{ii}, m{mm} y{yy} - { if (!valid(i, m, y)) throw Bad_date{}; } + class Date { // BAD: repetitive + int d; + Month m; + int y; + public: + Date(int ii, Month mm, year yy) + :i{ii}, m{mm} y{yy} + { if (!valid(i, m, y)) throw Bad_date{}; } - Date(int ii, Month mm) - :i{ii}, m{mm} y{current_year()} - { if (!valid(i, m, y)) throw Bad_date{}; } - // ... - }; + Date(int ii, Month mm) + :i{ii}, m{mm} y{current_year()} + { if (!valid(i, m, y)) throw Bad_date{}; } + // ... + }; The common action gets tedious to write and may accidentally not be common. ##### Example - class Date2 { - int d; - Month m; - int y; - public: - Date2(int ii, Month mm, year yy) - :i{ii}, m{mm} y{yy} - { if (!valid(i, m, y)) throw Bad_date{}; } + class Date2 { + int d; + Month m; + int y; + public: + Date2(int ii, Month mm, year yy) + :i{ii}, m{mm} y{yy} + { if (!valid(i, m, y)) throw Bad_date{}; } - Date2(int ii, Month mm) - :Date2{ii, mm, current_year()} {} - // ... - }; + Date2(int ii, Month mm) + :Date2{ii, mm, current_year()} {} + // ... + }; **See also**: If the "repeated action" is a simple initialization, consider [an in-class member initializer](#Rc-in-class-initializer). @@ -4264,25 +4248,25 @@ The common action gets tedious to write and may accidentally not be common. `std::vector` has a lot of tricky constructors, so if I want my own `vector`, I don't want to reimplement them: - class Rec { - // ... data and lots of nice constructors ... - }; + class Rec { + // ... data and lots of nice constructors ... + }; - class Oper : public Rec { - using Rec::Rec; - // ... no data members ... - // ... lots of nice utility functions ... - }; + class Oper : public Rec { + using Rec::Rec; + // ... no data members ... + // ... lots of nice utility functions ... + }; ##### Example, bad - struct Rec2 : public Rec { - int x; - using Rec::Rec; - }; + struct Rec2 : public Rec { + int x; + using Rec::Rec; + }; - Rec2 r {"foo", 7}; - int val = r.x; // uninitialized + Rec2 r {"foo", 7}; + int val = r.x; // uninitialized ##### Enforcement @@ -4302,23 +4286,23 @@ Types can be defined to move for logical as well as performance reasons. ##### Example - class Foo { - public: - Foo& operator=(const Foo& x) - { + class Foo { + public: + Foo& operator=(const Foo& x) + { auto tmp = x; // GOOD: no need to check for self-assignment (other than performance) - std::swap(*this, tmp); - return *this; - } - // ... - }; + std::swap(*this, tmp); + return *this; + } + // ... + }; - Foo a; - Foo b; - Foo f(); + Foo a; + Foo b; + Foo f(); - a = b; // assign lvalue: copy - a = f(); // assign rvalue: potentially move + a = b; // assign lvalue: copy + a = f(); // assign rvalue: potentially move ##### Note @@ -4328,30 +4312,31 @@ The `swap` implementation technique offers the [strong guarantee](???). But what if you can get significant better performance by not making a temporary copy? Consider a simple `Vector` intended for a domain where assignment of large, equal-sized `Vector`s is common. In this case, the copy of elements implied by the `swap` implementation technique could cause an order of magnitude increase in cost: - template - class Vector { - public: - Vector& operator=(const Vector&); - // ... - private: - T* elem; - int sz; - }; + template + class Vector { + public: + Vector& operator=(const Vector&); + // ... + private: + T* elem; + int sz; + }; + } - Vector& Vector::operator=(const Vector& a) - { - if (a.sz>sz) - { - // ... use the swap technique, it can't be bettered ... - return *this - } - // ... copy sz elements from *a.elem to elem ... - if (a.szsz) + { + // ... use the swap technique, it can't be bettered ... + return *this + } + // ... copy sz elements from *a.elem to elem ... + if (a.sz v = {3, 1, 4, 1, 5, 9}; - v = v; - // the value of v is still {3, 1, 4, 1, 5, 9} + std::vector v = {3, 1, 4, 1, 5, 9}; + v = v; + // the value of v is still {3, 1, 4, 1, 5, 9} ##### Note The default assignment generated from members that handle self-assignment correctly handles self-assignment. - struct Bar { - vector> v; - map m; - string s; - }; + struct Bar { + vector> v; + map m; + string s; + }; - Bar b; - // ... - b = b; // correct and efficient + Bar b; + // ... + b = b; // correct and efficient ##### Note You can handle self-assignment by explicitly testing for self-assignment, but often it is faster and more elegant to cope without such a test (e.g., [using `swap`](#Rc-swap)). - class Foo { - string s; - int i; - public: - Foo& operator=(const Foo& a); - // ... - }; + class Foo { + string s; + int i; + public: + Foo& operator=(const Foo& a); + // ... + }; - Foo& Foo::operator=(const Foo& a) // OK, but there is a cost - { - if (this==&a) return *this; - s = a.s; - i = a.i; - return *this; - } + Foo& Foo::operator=(const Foo& a) // OK, but there is a cost + { + if (this==&a) return *this; + s = a.s; + i = a.i; + return *this; + } This is obviously safe and apparently efficient. However, what if we do one self-assignment per million assignments? That's about a million redundant tests (but since the answer is essentially always the same, the computer's branch predictor will guess right essentially every time). Consider: - Foo& Foo::operator=(const Foo& a) // simpler, and probably much better - { - s = a.s; - i = a.i; - return *this; - } + Foo& Foo::operator=(const Foo& a) // simpler, and probably much better + { + s = a.s; + i = a.i; + return *this; + } `std::string` is safe for self-assignment and so are `int`. All the cost is carried by the (rare) case of self-assignment. @@ -4513,6 +4498,7 @@ Consider: ##### Enforcement Equivalent to what is done for [copy-assignment](#Rc-copy-assignment). + * (Simple) An assignment operator should not be virtual. Here be dragons! * (Simple) An assignment operator should return `T&` to enable chaining, not alternatives like `const T&` which interfere with composability and putting objects in containers. * (Moderate) A move assignment operator should (implicitly or explicitly) invoke all base and member move assignment operators. @@ -4525,34 +4511,34 @@ Equivalent to what is done for [copy-assignment](#Rc-copy-assignment). ##### Example - template - class X { // OK: value semantics - public: - X(); - X(X&& a); // move X - void modify(); // change the value of X - // ... - ~X() { delete[] p; } - private: - T* p; - int sz; - }; + template + class X { // OK: value semantics + public: + X(); + X(X&& a); // move X + void modify(); // change the value of X + // ... + ~X() { delete[] p; } + private: + T* p; + int sz; + }; - X::X(X&& a) - :p{a.p}, sz{a.sz} // steal representation - { - a.p = nullptr; // set to "empty" - a.sz = 0; - } + X::X(X&& a) + :p{a.p}, sz{a.sz} // steal representation + { + a.p = nullptr; // set to "empty" + a.sz = 0; + } - void use() - { - X x{}; - // ... - X y = std::move(x); - x = X{}; // OK - } // OK: x can be destroyed + void use() + { + X x{}; + // ... + X y = std::move(x); + x = X{}; // OK + } // OK: x can be destroyed ##### Note @@ -4575,21 +4561,21 @@ Unless there is an exceptionally strong reason not to, make `x=std::move(y); y=z ##### Example - class Foo { - string s; - int i; - public: - Foo& operator=(Foo&& a); - // ... - }; + class Foo { + string s; + int i; + public: + Foo& operator=(Foo&& a); + // ... + }; - Foo& Foo::operator=(Foo&& a) // OK, but there is a cost - { - if (this==&a) return *this; // this line is redundant - s = std::move(a.s); - i = a.i; - return *this; - } + Foo& Foo::operator=(Foo&& a) // OK, but there is a cost + { + if (this==&a) return *this; // this line is redundant + s = std::move(a.s); + i = a.i; + return *this; + } The one-in-a-million argument against `if (this==&a) return *this;` tests from the discussion of [self-assignment](#Rc-copy-self) is even more relevant for self-move. @@ -4605,11 +4591,11 @@ The ISO standard guarantees only a "valid but unspecified" state for the standar Here is a way to move a pointer without a test (imagine it as code in the implementation a move assignment): - // move from other.oter to this->ptr - T* temp = other.ptr; - other.ptr = nullptr; - delete ptr; - ptr = temp; + // move from other.oter to this->ptr + T* temp = other.ptr; + other.ptr = nullptr; + delete ptr; + ptr = temp; ##### Enforcement @@ -4625,31 +4611,31 @@ A non-throwing move will be used more efficiently by standard-library and langua ##### Example - template - class Vector { - // ... - Vector(Vector&& a) noexcept :elem{a.elem}, sz{a.sz} { a.sz=0; a.elem=nullptr; } - Vector& operator=(Vector&& a) noexcept { elem=a.elem; sz=a.sz; a.sz=0; a.elem=nullptr; } - //... - public: - T* elem; - int sz; - }; + template + class Vector { + // ... + Vector(Vector&& a) noexcept :elem{a.elem}, sz{a.sz} { a.sz=0; a.elem=nullptr; } + Vector& operator=(Vector&& a) noexcept { elem=a.elem; sz=a.sz; a.sz=0; a.elem=nullptr; } + //... + public: + T* elem; + int sz; + }; These copy operations do not throw. ##### Example, bad - template - class Vector2 { - // ... - Vector2(Vector2&& a) { *this = a; } // just use the copy - Vector2& operator=(Vector2&& a) { *this = a; } // just use the copy - //... - public: - T* elem; - int sz; - }; + template + class Vector2 { + // ... + Vector2(Vector2&& a) { *this = a; } // just use the copy + Vector2& operator=(Vector2&& a) { *this = a; } // just use the copy + //... + public: + T* elem; + int sz; + }; This `Vector2` is not just inefficient, but since a vector copy requires allocation, it can throw. @@ -4716,33 +4702,33 @@ A class with any virtual function should not have a copy constructor or copy ass ##### Example - class Tracer { - string message; - public: - Tracer(const string& m) : message{m} { cerr << "entering " << message <<'\n'; } - ~Tracer() { cerr << "exiting " << message <<'\n'; } + class Tracer { + string message; + public: + Tracer(const string& m) : message{m} { cerr << "entering " << message <<'\n'; } + ~Tracer() { cerr << "exiting " << message <<'\n'; } - Tracer(const Tracer&) = default; - Tracer& operator=(const Tracer&) = default; - Tracer(Tracer&&) = default; - Tracer& operator=(Tracer&&) = default; - }; + Tracer(const Tracer&) = default; + Tracer& operator=(const Tracer&) = default; + Tracer(Tracer&&) = default; + Tracer& operator=(Tracer&&) = default; + }; Because we defined the destructor, we must define the copy and move operations. The `=default` is the best and simplest way of doing that. ##### Example, bad - class Tracer2 { - string message; - public: - Tracer2(const string& m) : message{m} { cerr << "entering " << message <<'\n'; } - ~Tracer2() { cerr << "exiting " << message <<'\n'; } + class Tracer2 { + string message; + public: + Tracer2(const string& m) : message{m} { cerr << "entering " << message <<'\n'; } + ~Tracer2() { cerr << "exiting " << message <<'\n'; } - Tracer2(const Tracer2& a) : message{a.message} {} - Tracer2& operator=(const Tracer2& a) { message=a.message; } - Tracer2(Tracer2&& a) :message{a.message} {} - Tracer2& operator=(Tracer2&& a) { message=a.message; } - }; + Tracer2(const Tracer2& a) : message{a.message} {} + Tracer2& operator=(const Tracer2& a) { message=a.message; } + Tracer2(Tracer2&& a) :message{a.message} {} + Tracer2& operator=(Tracer2&& a) { message=a.message; } + }; Writing out the bodies of the copy and move operations is verbose, tedious, and error-prone. A compiler does it better. @@ -4758,43 +4744,43 @@ Writing out the bodies of the copy and move operations is verbose, tedious, and ##### Example - class Immortal { - public: - ~Immortal() = delete; // do not allow destruction - // ... - }; + class Immortal { + public: + ~Immortal() = delete; // do not allow destruction + // ... + }; - void use() - { - Immortal ugh; // error: ugh cannot be destroyed - Immortal* p = new Immortal{}; - delete p; // error: cannot destroy *p - } + void use() + { + Immortal ugh; // error: ugh cannot be destroyed + Immortal* p = new Immortal{}; + delete p; // error: cannot destroy *p + } ##### Example A `unique_ptr` can be moved, but not copied. To achieve that its copy operations are deleted. To avoid copying it is necessary to `=delete` its copy operations from lvalues: - template > class unique_ptr { - public: - // ... - constexpr unique_ptr() noexcept; - explicit unique_ptr(pointer p) noexcept; - // ... - unique_ptr(unique_ptr&& u) noexcept; // move constructor - // ... - unique_ptr(const unique_ptr&) = delete; // disable copy from lvalue - // ... - }; + template > class unique_ptr { + public: + // ... + constexpr unique_ptr() noexcept; + explicit unique_ptr(pointer p) noexcept; + // ... + unique_ptr(unique_ptr&& u) noexcept; // move constructor + // ... + unique_ptr(const unique_ptr&) = delete; // disable copy from lvalue + // ... + }; - unique_ptr make(); // make "something" and return it by moving + unique_ptr make(); // make "something" and return it by moving - void f() - { - unique_ptr pi {}; - auto pi2 {pi}; // error: no move constructor from lvalue - auto pi3 {make()}; // OK, move: the result of make() is an rvalue - } + void f() + { + unique_ptr pi {}; + auto pi2 {pi}; // error: no move constructor from lvalue + auto pi3 {make()}; // OK, move: the result of make() is an rvalue + } ##### Enforcement @@ -4810,28 +4796,28 @@ Worse, a direct or indirect call to an unimplemented pure virtual function from ##### Example, bad - class base { - public: - virtual void f() = 0; // not implemented - virtual void g(); // implemented with base version - virtual void h(); // implemented with base version - }; + class base { + public: + virtual void f() = 0; // not implemented + virtual void g(); // implemented with base version + virtual void h(); // implemented with base version + }; - class derived : public base { - public: - void g() override; // provide derived implementation - void h() final; // provide derived implementation + class derived : public base { + public: + void g() override; // provide derived implementation + void h() final; // provide derived implementation - derived() - { - f(); // BAD: attempt to call an unimplemented virtual function + derived() + { + f(); // BAD: attempt to call an unimplemented virtual function - g(); // BAD: will call derived::g, not dispatch further virtually - derived::g(); // GOOD: explicitly state intent to call only the visible version + g(); // BAD: will call derived::g, not dispatch further virtually + derived::g(); // GOOD: explicitly state intent to call only the visible version - h(); // ok, no qualification needed, h is final - } - }; + h(); // ok, no qualification needed, h is final + } + }; Note that calling a specific explicitly qualified function is not a virtual call even if the function is `virtual`. @@ -4878,12 +4864,12 @@ Providing a nonmember `swap` function in the same namespace as your type for cal ##### Example, bad - void swap(My_vector& x, My_vector& y) - { - auto tmp = x; // copy elements - x = y; - y = tmp; - } + void swap(My_vector& x, My_vector& y) + { + auto tmp = x; // copy elements + x = y; + y = tmp; + } This is not just slow, but if a memory allocation occur for the elements in `tmp`, this `swap` may throw and would make STL algorithms fail is used with them. @@ -4911,21 +4897,21 @@ If a `swap` tries to exit with an exception, it's a bad design error and the pro ##### Example - class X { - string name; - int number; - }; + class X { + string name; + int number; + }; - bool operator==(const X& a, const X& b) noexcept { return a.name==b.name && a.number==b.number; } + bool operator==(const X& a, const X& b) noexcept { return a.name==b.name && a.number==b.number; } ##### Example, bad - class B { - string name; - int number; - bool operator==(const B& a) const { return name==a.name && number==a.number; } - // ... - }; + class B { + string name; + int number; + bool operator==(const B& a) const { return name==a.name && number==a.number; } + // ... + }; `B`'s comparison accepts conversions for its second operand, but not its first. @@ -4946,29 +4932,29 @@ The alternative is to make two failure states compare equal and any valid state ##### Example, bad - class B { - string name; - int number; - virtual bool operator==(const B& a) const { return name==a.name && number==a.number; } - // ... - }; + class B { + string name; + int number; + virtual bool operator==(const B& a) const { return name==a.name && number==a.number; } + // ... + }; // `B`'s comparison accepts conversions for its second operand, but not its first. - class D :B { - char character; - virtual bool operator==(const D& a) const { return name==a.name && number==a.number && character==a.character; } - // ... - }; + class D :B { + char character; + virtual bool operator==(const D& a) const { return name==a.name && number==a.number && character==a.character; } + // ... + }; - B b = ... - D d = ... - b==d; // compares name and number, ignores d's character - d==b; // error: no == defined - D d2; - d==d2; // compares name, number, and character - B& b2 = d2; - b2==d; // compares name and number, ignores d2's and d's character + B b = ... + D d = ... + b==d; // compares name and number, ignores d's character + d==b; // error: no == defined + D d2; + d==d2; // compares name, number, and character + B& b2 = d2; + b2==d; // compares name and number, ignores d2's and d's character Of course there are way of making `==` work in a hierarchy, but the naive approaches do not scale @@ -4984,7 +4970,7 @@ Of course there are way of making `==` work in a hierarchy, but the naive approa ##### Example - ??? + ??? ##### Enforcement @@ -4998,7 +4984,7 @@ Of course there are way of making `==` work in a hierarchy, but the naive approa ##### Example - ??? + ??? ##### Enforcement @@ -5084,28 +5070,28 @@ Do *not* use inheritance when simply having a data member will do. Usually this ##### Example - ??? Good old Shape example? + ??? Good old Shape example? ##### Example, bad Do *not* represent non-hierarchical domain concepts as class hierarchies. - template - class Container { - public: - // list operations: - virtual T& get() = 0; - virtual void put(T&) = 0; - virtual void insert(Position) = 0; - // ... - // vector operations: - virtual T& operator[](int) = 0; - virtual void sort() = 0; - // ... - // tree operations: - virtual void balance() = 0; - // ... - }; + template + class Container { + public: + // list operations: + virtual T& get() = 0; + virtual void put(T&) = 0; + virtual void insert(Position) = 0; + // ... + // vector operations: + virtual T& operator[](int) = 0; + virtual void sort() = 0; + // ... + // tree operations: + virtual void balance() = 0; + // ... + }; Here most overriding classes cannot implement most of the functions required in the interface well. Thus the base class becomes an implementation burden. @@ -5127,7 +5113,7 @@ not using this (over)general interface in favor of a particular interface found ##### Example - ??? + ??? ##### Enforcement @@ -5141,11 +5127,11 @@ not using this (over)general interface in favor of a particular interface found ##### Example - ??? + ??? ##### Enforcement -??? + ??? ## C.hierclass: Designing classes in a hierarchy: @@ -5157,7 +5143,7 @@ not using this (over)general interface in favor of a particular interface found ##### Example - ??? + ??? ##### Exceptions @@ -5177,19 +5163,19 @@ Flag abstract classes with constructors. ##### Example, bad - struct B { - // ... no destructor ... - }; + struct B { + // ... no destructor ... + }; - struct D : B { // bad: class with a resource derived from a class without a virtual destructor - string s {"default"}; - }; + struct D : B { // bad: class with a resource derived from a class without a virtual destructor + string s {"default"}; + }; - void use() - { - B* p = new D; - delete p; // leak the string - } + void use() + { + B* p = new D; + delete p; // leak the string + } ##### Note @@ -5208,19 +5194,19 @@ There are people who don't follow this rule because they plan to use a class onl ##### Example, bad - struct B { - void f1(int); - virtual void f2(int); - virtual void f3(int); - // ... - }; + struct B { + void f1(int); + virtual void f2(int); + virtual void f3(int); + // ... + }; - struct D : B { - void f1(int); // warn: D::f1() hides B::f1() - void f2(int); // warn: no explicit override - void f3(double); // warn: D::f3() hides B::f3() - // ... - }; + struct D : B { + void f1(int); // warn: D::f1() hides B::f1() + void f2(int); // warn: no explicit override + void f3(double); // warn: D::f3() hides B::f3() + // ... + }; ##### Enforcement @@ -5235,7 +5221,7 @@ There are people who don't follow this rule because they plan to use a class onl ##### Example - ??? + ??? ##### Enforcement @@ -5249,15 +5235,15 @@ There are people who don't follow this rule because they plan to use a class onl ##### Example - class base { - public: - virtual base* clone() =0; - }; + class base { + public: + virtual base* clone() =0; + }; - class derived : public base { - public: - derived* clone() override; - }; + class derived : public base { + public: + derived* clone() override; + }; Note that because of language rules, the covariant return type cannot be a smart pointer. @@ -5274,24 +5260,24 @@ Note that because of language rules, the covariant return type cannot be a smart ##### Example - class point { - int x; - int y; - public: - point(int xx, int yy) : x{xx}, y{yy} { } - int get_x() { return x; } - void set_x(int xx) { x = xx; } - int get_y() { return y; } - void set_y(int yy) { y = yy; } - // no behavioral member functions - }; + class point { + int x; + int y; + public: + point(int xx, int yy) : x{xx}, y{yy} { } + int get_x() { return x; } + void set_x(int xx) { x = xx; } + int get_y() { return y; } + void set_y(int yy) { y = yy; } + // no behavioral member functions + }; Consider making such a class a `struct` -- that is, a behaviorless bunch of variables, all public data and no member functions. - struct point { - int x = 0; - int y = 0; - }; + struct point { + int x = 0; + int y = 0; + }; ##### Note @@ -5311,15 +5297,15 @@ A virtual function ensures code replication in a templated hierarchy. ##### Example, bad - template - class Vector { - public: - // ... - virtual int size() const { return sz; } // bad: what good could a derived class do? - private: - T* elem; // the elements - int sz; // number of elements - }; + template + class Vector { + public: + // ... + virtual int size() const { return sz; } // bad: what good could a derived class do? + private: + T* elem; // the elements + int sz; // number of elements + }; This kind of "vector" isn't meant to be used as a base class at all. @@ -5338,7 +5324,7 @@ This kind of "vector" isn't meant to be used as a base class at all. ##### Example - ??? + ??? ##### Note @@ -5356,7 +5342,7 @@ Flag classes with `protected` data. ##### Example - ??? + ??? ##### Enforcement @@ -5370,7 +5356,7 @@ Flag any class that has data members with different access levels. ##### Example - ??? + ??? ##### Note @@ -5393,7 +5379,7 @@ Such interfaces are typically abstract classes. ##### Example - ??? + ??? ##### Note @@ -5411,7 +5397,7 @@ This a relatively rare use because implementation can often be organized into a ##### Example - ??? + ??? ##### Note @@ -5425,11 +5411,11 @@ This a relatively rare use because implementation can often be organized into a ##### Reason - ??? +??? ##### Example - ??? + ??? ## C.hier-access: Accessing objects in a hierarchy @@ -5441,21 +5427,21 @@ This a relatively rare use because implementation can often be organized into a ##### Example - struct B { int a; virtual int f(); }; - struct D : B { int b; int f() override; }; + struct B { int a; virtual int f(); }; + struct D : B { int b; int f() override; }; - void use(B b) - { - D d; - B b2 = d; // slice - B b3 = b; - } + void use(B b) + { + D d; + B b2 = d; // slice + B b3 = b; + } - void use2() - { - D d; - use(d); // slice - } + void use2() + { + D d; + use(d); // slice + } Both `d`s are sliced. @@ -5463,11 +5449,11 @@ Both `d`s are sliced. You can safely access a named polymorphic object in the scope of its definition, just don't slice it. - void use3() - { - D d; - d.f(); // OK - } + void use3() + { + D d; + d.f(); // OK + } ##### Enforcement @@ -5481,25 +5467,25 @@ Flag all slicing. ##### Example - struct B { // an interface - virtual void f(); - virtual void g(); - }; + struct B { // an interface + virtual void f(); + virtual void g(); + }; - struct D : B { // a wider interface - void f() override; - virtual void h(); - }; + struct D : B { // a wider interface + void f() override; + virtual void h(); + }; - void user(B* pb) - { - if (D* pd = dynamic_cast(pb)) { - // ... use D's interface ... - } - else { - // .. make do with B's interface ... - } - } + void user(B* pb) + { + if (D* pd = dynamic_cast(pb)) { + // ... use D's interface ... + } + else { + // .. make do with B's interface ... + } + } ##### Note @@ -5528,7 +5514,7 @@ Flag all uses of `static_cast` for downcasts, including C-style casts that perfo ##### Example - ??? + ??? ##### Enforcement @@ -5542,7 +5528,7 @@ Flag all uses of `static_cast` for downcasts, including C-style casts that perfo ##### Example - ??? + ??? ##### Enforcement @@ -5556,13 +5542,13 @@ Flag all uses of `static_cast` for downcasts, including C-style casts that perfo ##### Example - void use(int i) - { - auto p = new int {7}; // bad: initialize local pointers with new - auto q = make_unique(9); // ok: guarantee the release of the memory allocated for 9 - if(0(9); // ok: guarantee the release of the memory allocated for 9 + if(0 p {new{7}); // OK: but repetitive + unique_ptr p {new{7}); // OK: but repetitive - auto q = make_unique(7); // Better: no repetition of Foo + auto q = make_unique(7); // Better: no repetition of Foo ##### Enforcement @@ -5595,9 +5581,9 @@ It also gives an opportunity to eliminate a separate allocation for the referenc ##### Example - shared_ptr p {new{7}); // OK: but repetitive; and separate allocations for the Foo and shared_ptr's use count + shared_ptr p {new{7}); // OK: but repetitive; and separate allocations for the Foo and shared_ptr's use count - auto q = make_shared(7); // Better: no repetition of Foo; one object + auto q = make_shared(7); // Better: no repetition of Foo; one object ##### Enforcement @@ -5612,16 +5598,16 @@ It also gives an opportunity to eliminate a separate allocation for the referenc ##### Example - struct B { int x; }; - struct D : B { int y; }; + struct B { int x; }; + struct D : B { int y; }; - void use(B*); + void use(B*); - D a[] = { {1, 2}, {3, 4}, {5, 6} }; - B* p = a; // bad: a decays to &a[0] which is converted to a B* - p[1].x = 7; // overwrite D[0].y + D a[] = { {1, 2}, {3, 4}, {5, 6} }; + B* p = a; // bad: a decays to &a[0] which is converted to a B* + p[1].x = 7; // overwrite D[0].y - use(a); // bad: a decays to &a[0] which is converted to a B* + use(a); // bad: a decays to &a[0] which is converted to a B* ##### Enforcement @@ -5650,7 +5636,7 @@ Overload rule summary: ##### Example, bad - X operator+(X a, X b) { return a.v-b.v; } // bad: makes + subtract + X operator+(X a, X b) { return a.v-b.v; } // bad: makes + subtract ???. Non-member operators: namespace-level definition (traditional?) vs friend definition (as used by boost.operator, limits lookup to ADL only) @@ -5667,7 +5653,7 @@ Unless you use a non-member function for (say) `==`, `a==b` and `b==a` will be s ##### Example - bool operator==(Point a, Point b) { return a.x==b.x && a.y==b.y; } + bool operator==(Point a, Point b) { return a.x==b.x && a.y==b.y; } ##### Enforcement @@ -5683,15 +5669,15 @@ Flag member operator functions. Consider - void print(int a); - void print(int a, int base); - void print(const string&); + void print(int a); + void print(int a, int base); + void print(const string&); These three functions all prints their arguments (appropriately). Conversely - void print_int(int a); - void print_based(int a, int base); - void print_string(const string&); + void print_int(int a); + void print_based(int a, int base); + void print_string(const string&); These three functions all prints their arguments (appropriately). Adding to the name just introduced verbosity and inhibits generic code. @@ -5709,13 +5695,13 @@ These three functions all prints their arguments (appropriately). Adding to the Consider - void open_gate(Gate& g); // remove obstacle from garage exit lane - void fopen(const char*name, const char* mode); // open file + void open_gate(Gate& g); // remove obstacle from garage exit lane + void fopen(const char*name, const char* mode); // open file The two operations are fundamentally different (and unrelated) so it is good that their names differ. Conversely: - void open(Gate& g); // remove obstacle from garage exit lane - void open(const char*name, const char* mode ="r"); // open file + void open(Gate& g); // remove obstacle from garage exit lane + void open(const char*name, const char* mode ="r"); // open file The two operations are still fundamentally different (and unrelated) but the names have been reduced to their (common) minimum, opening opportunities for confusion. Fortunately, the type system will catch many such mistakes. @@ -5743,23 +5729,23 @@ just to gain a minor convenience. ##### Example, bad - class String { // handle ownership and access to a sequence of characters - // ... - String(czstring p); // copy from *p to *(this->elem) - // ... - operator zstring() { return elem; } - // ... - }; + class String { // handle ownership and access to a sequence of characters + // ... + String(czstring p); // copy from *p to *(this->elem) + // ... + operator zstring() { return elem; } + // ... + }; - void user(zstring p) - { - if (*p=="") { - String s {"Trouble ahead!"}; - // ... - p = s; - } - // use p - } + void user(zstring p) + { + if (*p=="") { + String s {"Trouble ahead!"}; + // ... + p = s; + } + // use p + } The string allocated for `s` and assigned to `p` is destroyed before it can be used. @@ -5775,14 +5761,14 @@ Flag all conversion operators. ##### Example - void f(int); - void f(double); - auto f = [](char); // error: cannot overload variable and function + void f(int); + void f(double); + auto f = [](char); // error: cannot overload variable and function - auto g = [](int) { /* ... */ }; - auto g = [](double) { /* ... */ }; // error: cannot overload variables + auto g = [](int) { /* ... */ }; + auto g = [](double) { /* ... */ }; // error: cannot overload variables - auto h = [](auto) { /* ... */ }; // OK + auto h = [](auto) { /* ... */ }; // OK ##### Enforcement @@ -5810,7 +5796,7 @@ Union rule summary: ##### Example - ??? + ??? ##### Enforcement @@ -5828,7 +5814,7 @@ Union rule summary: ##### Example - ??? + ??? ##### Enforcement @@ -5838,11 +5824,11 @@ Union rule summary: ##### Reason - ??? +??? ##### Example - ??? + ??? ##### Enforcement @@ -5870,7 +5856,7 @@ Enumeration rule summary: ##### Example - ??? + ??? ##### Enforcement @@ -5884,7 +5870,7 @@ Enumeration rule summary: ##### Example - ??? + ??? ##### Enforcement @@ -5898,7 +5884,7 @@ Enumeration rule summary: ##### Example - ??? + ??? ##### Enforcement @@ -5912,7 +5898,7 @@ Enumeration rule summary: ##### Example - ??? + ??? ##### Enforcement @@ -5926,7 +5912,7 @@ Enumeration rule summary: ##### Example - ??? + ??? ##### Enforcement @@ -5936,11 +5922,11 @@ Enumeration rule summary: ##### Reason - ??? +??? ##### Example - ??? + ??? ##### Enforcement @@ -6063,22 +6049,22 @@ Such containers and views hold sufficient information to do range checking. ##### Example, bad - void f(int* p, int n) // n is the number of elements in p[] - { - // ... - p[2] = 7; // bad: subscript raw pointer - // ... - } + void f(int* p, int n) // n is the number of elements in p[] + { + // ... + p[2] = 7; // bad: subscript raw pointer + // ... + } The compiler does not read comments, and without reading other code you do not know whether `p` really points to `n` elements. Use an `array_view` instead. ##### Example - void g(int* p, int fmt) // print *p using format #fmt - { - // ... uses *p and p[0] only ... - } + void g(int* p, int fmt) // print *p using format #fmt + { + // ... uses *p and p[0] only ... + } **Exception**: C-style strings are passed as single pointers to a zero-terminated sequence of characters. Use `zstring` rather than `char*` to indicate that you rely on that convention. @@ -6103,34 +6089,34 @@ We want owning pointers identified so that we can reliably and efficiently delet ##### Example - void f() - { - int* p1 = new int{7}; // bad: raw owning pointer - auto p2 = make_unique(7); // OK: the int is owned by a unique pointer - // ... - } + void f() + { + int* p1 = new int{7}; // bad: raw owning pointer + auto p2 = make_unique(7); // OK: the int is owned by a unique pointer + // ... + } The `unique_ptr` protects against leaks by guaranteeing the deletion of its object (even in the presence of exceptions). The `T*` does not. ##### Example - template - class X { - // ... - public: - T* p; // bad: it is unclear whether p is owning or not - T* q; // bad: it is unclear whether q is owning or not - }; + template + class X { + // ... + public: + T* p; // bad: it is unclear whether p is owning or not + T* q; // bad: it is unclear whether q is owning or not + }; We can fix that problem by making ownership explicit: - template - class X2 { - // ... - public: - owner p; // OK: p is owning - T* q; // OK: q is not owning - }; + template + class X2 { + // ... + public: + owner p; // OK: p is owning + T* q; // OK: q is not owning + }; ##### Note @@ -6149,30 +6135,30 @@ For example, if an `owner` is a member of a class, that class better have a d Returning a (raw) pointer imposes a life-time management burden on the caller; that is, who deletes the pointed-to object? - Gadget* make_gadget(int n) - { - auto p = new Gadget{n}; - // ... - return p; + Gadget* make_gadget(int n) + { + auto p = new Gadget{n}; + // ... + return p; } - void caller(int n) - { - auto p = make_gadget(n); // remember to delete p - // ... - delete p; - } + void caller(int n) + { + auto p = make_gadget(n); // remember to delete p + // ... + delete p; + } In addition to suffering from then problem from [leak](#???), this adds a spurious allocation and deallocation operation, and is needlessly verbose. If Gadget is cheap to move out of a function (i.e., is small or has an efficient move operation), just return it "by value:' - Gadget make_gadget(int n) - { - Gadget g{n}; - // ... - return g; - } + Gadget make_gadget(int n) + { + Gadget g{n}; + // ... + return g; + } ##### Note @@ -6200,12 +6186,12 @@ We want owners identified so that we can reliably and efficiently delete the obj ##### Example - void f() - { - int& r = *new int{7}; // bad: raw owning reference - // ... - delete &r; // bad: violated the rule against deleting raw pointers - } + void f() + { + int& r = *new int{7}; // bad: raw owning reference + // ... + delete &r; // bad: violated the rule against deleting raw pointers + } **See also**: [The raw pointer rule](#Rr-ptr) @@ -6225,20 +6211,20 @@ The members of a scoped object are themselves scoped and the scoped object's con the following example is inefficient (because it has unnecessary allocation and deallocation), vulnerable to exception throws and returns in the "¦ part (leading to leaks), and verbose: - void some_function(int n) - { - auto p = new Gadget{n}; - // ... - delete p; - } + void some_function(int n) + { + auto p = new Gadget{n}; + // ... + delete p; + } Instead, use a local variable: - void some_function(int n) - { - Gadget g{n}; - // ... - } + void some_function(int n) + { + Gadget g{n}; + // ... + } ##### Enforcement @@ -6336,24 +6322,24 @@ If you have a naked `new`, you probably need a naked `delete` somewhere, so you ##### Example, bad - void f(const string& name) - { - FILE* f = fopen(name, "r"); // open the file - vector buf(1024); - auto _ = finally([] { fclose(f); } // remember to close the file - // ... - } + void f(const string& name) + { + FILE* f = fopen(name, "r"); // open the file + vector buf(1024); + auto _ = finally([] { fclose(f); } // remember to close the file + // ... + } The allocation of `buf` may fail and leak the file handle. ##### Example - void f(const string& name) - { - ifstream {name, "r"}; // open the file - vector buf(1024); - // ... - } + void f(const string& name) + { + ifstream {name, "r"}; // open the file + vector buf(1024); + // ... + } The use of the file handle (in `ifstream`) is simple, efficient, and safe. @@ -6449,13 +6435,13 @@ Flag incomplete pairs. Consider - void f() - { - X x; - X* p1 { new X }; // see also ??? - unique_ptr p2 { new X }; // unique ownership; see also ??? - shared_ptr p3 { new X }; // shared ownership; see also ??? - } + void f() + { + X x; + X* p1 { new X }; // see also ??? + unique_ptr p2 { new X }; // unique ownership; see also ??? + shared_ptr p3 { new X }; // shared ownership; see also ??? + } This will leak the object used to initialize `p1` (only). @@ -6503,8 +6489,8 @@ This is more efficient Consider - shared_ptr p1 { new X{2} }; // bad - auto p = make_shared(2); // good + shared_ptr p1 { new X{2} }; // bad + auto p = make_shared(2); // good The `make_shared()` version mentions `X` only once, so it is usually shorter (as well as faster) than the version with the explicit `new`. @@ -6535,7 +6521,7 @@ be able to destroy a cyclic structure. ##### Example - ??? + ??? ##### Note @@ -6868,18 +6854,18 @@ It is available as part of all C++ Implementations. ##### Example - auto sum = accumulate(begin(a), end(a), 0.0); // good + 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 + 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 read1(istream& is) // good - { - vector res; - for (string s; is>>s; ) - res.push_back(s); - return res; - } + vector read1(istream& is) // good + { + vector 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(ps)) { // good: pc is local to if-statement - // ...deal with Circle ... - } - else { - // ... handle error ... - } - } + if (auto pc = dynamic_cast(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 ... - } + 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 measure too long anyway, but the point is that the 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 ide to factor out the read: - void fill_record(Record& r, const string& name) - { - string fn = name+".txt"; - ifstream is {fn}; - Record r; - is >> r; - } + void fill_record(Record& r, const string& name) + { + string fn = name+".txt"; + ifstream is {fn}; + Record r; + is >> r; + } - void use(const string& name) - { - Record r; - fill_record(r, name); - // ... 200 lines of code ... - } + void use(const string& name) + { + Record r; + fill_record(r, name); + // ... 200 lines of code ... + } I am assuming that `Record` is large and doesn't have a good move operation so that an out-parameter is preferable to returning a `Record`. @@ -6998,22 +6984,22 @@ I am assuming that `Record` is large and doesn't have a good move operation so t ##### Example - void use() - { - for (string s; cin>>s; ) - v.push_back(s); + 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 - //* ... - } + for (int i=0; i<20; ++i) { // good: i is local to for-loop + //* ... + } - if (auto pc = dynamic_cast(ps)) { // good: pc is local to if-statement - // ...deal with Circle ... - } - else { - // ... handle error ... - } - } + if (auto pc = dynamic_cast(ps)) { // good: pc is local to if-statement + // ...deal with Circle ... + } + else { + // ... handle error ... + } + } ##### Enforcement @@ -7030,24 +7016,24 @@ I am assuming that `Record` is large and doesn't have a good move operation so t Conventional short, local names increase readability: - template // good - void print(ostream& os, const vector& v) - { - for (int i = 0; i // good + void print(ostream& os, const vector& v) + { + for (int i = 0; i // bad: verbose, hard to read - void print(ostream& target_stream, const vector& current_vector) - { - for (int current_element_index = 0; - current_element_index // bad: verbose, hard to read + void print(ostream& target_stream, const vector& current_vector) + { + for (int current_element_index = 0; + current_element_index - bool any_of(InputIterator first, InputIterator last, Predicate pred); + template + bool any_of(InputIterator first, InputIterator last, Predicate pred); or better using concepts - bool any_of(InputIterator first, InputIterator last, Predicate pred); + bool any_of(InputIterator first, InputIterator last, Predicate pred); ##### Example - double scalbn(double x, int n); // OK: x*pow(FLT_RADIX, n); FLT_RADIX is usually 2 + 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 - ); + double scalbn( // better: x*pow(FLT_RADIX, n); FLT_RADIX is usually 2 + double x, // base value + int n // exponent + ); or - double scalbn(double base, int exponent); // better: base*pow(FLT_RADIX, exponent); FLT_RADIX is usually 2 + double scalbn(double base, int exponent); // better: base*pow(FLT_RADIX, exponent); FLT_RADIX is usually 2 ##### Enforcement @@ -7186,32 +7172,32 @@ Flag non-function arguments with multiple declarators involving declarator opera Consider - auto p = v.begin(); // vector::iterator - auto s = v.size(); - auto h = t.future(); - auto q = new int[s]; - auto f = [](int x){ return x+10; }; + auto p = v.begin(); // vector::iterator + auto s = v.size(); + auto h = t.future(); + auto q = new 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 - auto Container::first() -> Iterator; // Container::Iterator + template + auto Container::first() -> Iterator; // Container::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 (obviously) - auto x{1}; // x is an int (after correction of the C++14 standard; initializer_list in C++11) + auto lst = { 1, 2, 3 }; // lst is an initializer list (obviously) + auto x{1}; // x is an int (after correction of the C++14 standard; initializer_list in C++11) ##### Note When concepts become available, we can (and should) be more specific about the type we are deducing: - // ... - ForwardIterator p = algo(x, y, z); + // ... + ForwardIterator p = algo(x, y, z); ##### Enforcement @@ -7225,30 +7211,30 @@ Flag redundant repetition of type names in a declaration. ##### Example - void use(int arg) // bad: uninitialized variable - { - int i; - // ... - i = 7; // initialize i - } + void use(int arg) // bad: uninitialized variable + { + int i; + // ... + 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 - // ... - } + void use(int arg) // OK + { + int i = 7; // OK: initialized + string s; // OK: default initialized + // ... + } ##### Exception It 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 may 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 - f.read(buf, max); + constexpr int max = 8*1024; + int buf[max]; // OK, but suspicious + 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. @@ -7259,20 +7245,20 @@ However, such examples do tend to leave uninitialized variables accessible, so t When feasible use a library function that is know not to overflow. For example: - string s; // s is default initialized to "" - cin>>s; // s expands to hold the string + 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; + 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 - // ... - cin>>i; + int i2 = 0; // better + // ... + cin>>i; A good optimizer should know about input operations and eliminate the redundant operation. @@ -7281,28 +7267,28 @@ A good optimizer should know about input operations and eliminate the redundant Sometimes, we want to initialize a set of variables with a call to a function that returns several values. That can lead to uninitialized variables (exceptly as for input operations): - error_code ec; - Value v; - tie(ec, v) = get_value(); // get_value() returns a pair + error_code ec; + Value v; + tie(ec, v) = get_value(); // get_value() returns a pair ##### 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 - ec = p.first; - return p.second; - }; + error_code ec; + Value v = [&]() { + auto p = get_value(); // get_value() returns a pair + ec = p.first; + return p.second; + }; or maybe - Value v = []() { - auto p = get_value(); // get_value() returns a pair - if (p.first) throw Bad_value{p.first}; - return p.second; - }; + Value v = []() { + auto p = get_value(); // get_value() returns a pair + if (p.first) throw Bad_value{p.first}; + return p.second; + }; **See also**: [ES.28](#Res-lambda-init) @@ -7320,9 +7306,9 @@ or maybe ##### Example - int x = 7; - // ... no use of x here ... - ++x; + int x = 7; + // ... no use of x here ... + ++x; ##### Enforcement @@ -7336,26 +7322,26 @@ Flag declaration that distant from their first use. ##### Example, bad - string s; - // ... no use of s here ... - s = "what a waste"; + string s; + // ... no use of s here ... + s = "what a waste"; ##### Example, bad - SomeLargeType var; // ugly CaMeLcAsEvArIaBlE + SomeLargeType var; // ugly 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; - } + 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 + // 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. @@ -7383,8 +7369,8 @@ For initializers of moderate complexity, including for `const` variables, consid For containers, there is a tradition for using `{...}` for a list of elements and `(...)` for sizes: - vector v1(10); // vector of 10 elements with the default value 0 - vector v2 {10}; // vector of 1 element with the value 10 + vector v1(10); // vector of 10 elements with the default value 0 + vector v2 {10}; // vector of 1 element with the value 10 ##### Note @@ -7392,51 +7378,51 @@ For containers, there is a tradition for using `{...}` for a list of elements an ##### Example - int x {7.9}; // error: narrowing - int y = 7.9; // OK: y becomes 7. Hope for a compiler warning + int x {7.9}; // error: narrowing + int y = 7.9; // OK: y becomes 7. Hope for a compiler warning ##### Note `{}` initialization can be used for all initialization; other forms of initialization can't: - auto p = new vector {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 - // ... - }; + auto p = new vector {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 + // ... + }; ##### Note Initialization of a variable declared `auto` with a single value `{v}` surprising results until recently: - auto x1 {7}; // x1 is sn int with the value 7 - auto x2 = {7}; // x2 is and initializer_int with an element 7 + auto x1 {7}; // x1 is sn int with the value 7 + auto x2 = {7}; // x2 is and initializer_int with an element 7 - auto x11 {7, 8}; // error: two initializers - auto x22 = {7, 8}; // x2 is and initializer_int with elements 7 and 8 + auto x11 {7, 8}; // error: two initializers + auto x22 = {7, 8}; // x2 is and initializer_int with elements 7 and 8 ##### Exception Use `={...}` if you really want an `initializer_list` - auto fib10 = {0, 1, 2, 3, 5, 8, 13, 25, 38, 63}; // fib10 is a list + auto fib10 = {0, 1, 2, 3, 5, 8, 13, 25, 38, 63}; // fib10 is a list ##### Example - template - void f() - { - T x1(1); // T initialized with 1 - T x0(); // bad: function declaration (often a mistake) + template + 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 - // ... - } + T y1 {1}; // T initialized with 1 + T y0 {}; // default initialized T + // ... + } **See also**: [Discussion](#???) @@ -7455,14 +7441,14 @@ Tricky. ##### Example - void use(bool leak) - { - auto p1 = make_unique(7); // OK - int* p2 = new int{7}; // bad: might leak - // ... - if (leak) return; - // ... - } + void use(bool leak) + { + auto p1 = make_unique(7); // OK + int* p2 = new int{7}; // bad: might leak + // ... + if (leak) return; + // ... + } If `leak==true` the object pointer to by `p2` is leaked and the object pointed to by `p1` is not. @@ -7478,12 +7464,12 @@ Look for raw pointers that are targets of `new`, `malloc()`, or functions that m ##### 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? - // ... - } + 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 @@ -7497,12 +7483,12 @@ Look to see if a variable is actually mutated, and flag it if not. Unfortunately ##### Example, bad - void use() - { - int i; - for (i=0; i<20; ++i) { /* ... */ } - for (i=0; i<200; ++i) { /* ... */ } // bad: i recycled - } + void use() + { + int i; + for (i=0; i<20; ++i) { /* ... */ } + for (i=0; i<200; ++i) { /* ... */ } // bad: i recycled + } ##### Enforcement @@ -7517,15 +7503,15 @@ They are not confused with non-standard extensions of built-in arrays. ##### Example, bad - const int n = 7; - int m = 9; + const int n = 7; + int m = 9; - void f() - { - int a1[n]; - int a2[m]; // error: not ISO C++ - // ... - } + void f() + { + int a1[n]; + int a2[m]; // error: not ISO C++ + // ... + } ##### Note @@ -7537,15 +7523,15 @@ The definition of `a2` is C but not C++ and is considered a security risk ##### Example - const int n = 7; - int m = 9; + const int n = 7; + int m = 9; - void f() - { - array a1; - stack_array a2(m); - // ... - } + void f() + { + array a1; + stack_array a2(m); + // ... + } ##### Enforcement @@ -7560,31 +7546,31 @@ The definition of `a2` is C but not C++ and is considered a security risk ##### 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 + 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; - }(); + 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; + }(); ##### Example - string var = [&]{ - if (!in) return ""; // default - string s; - for (char c : in>>c) - s += toupper(c); - return s; - }(); // note () + string var = [&]{ + if (!in) return ""; // default + string s; + for (char c : in>>c) + s += toupper(c); + return s; + }(); // note () 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. @@ -7612,7 +7598,7 @@ Macros complicates tool building. ##### Example, bad - #define Case break; case /* 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. @@ -7636,13 +7622,13 @@ Macros complicates tool building. ##### Example, bad - #define PI 3.14 - #define SQUARE(a, b) (a*b) + #define PI 3.14 + #define SQUARE(a, b) (a*b) Even if we hadn't left a well-know bug in `SQUARE` there are much better behaved alternatives; for example: - constexpr double pi = 3.14; - template T square(T a, T b) { return a*b; } + constexpr double pi = 3.14; + template T square(T a, T b) { return a*b; } ##### Enforcement @@ -7656,9 +7642,9 @@ Scream when you see a macro that isn't just use for source control (e.g., `#ifde ##### Example - #define forever for(;;) /* very BAD */ + #define forever for(;;) /* very BAD */ - #define FOREVER for(;;) /* Still evil, but at least visible to humans */ + #define FOREVER for(;;) /* Still evil, but at least visible to humans */ ##### Enforcement @@ -7672,7 +7658,7 @@ Scream when you see a lower case macro. ##### Example - ??? + ??? **Alternative**: Overloading. Templates. Variadic templates. @@ -7698,23 +7684,23 @@ Statements control the flow of control (except for function calls and exception ##### Example - void use(int n) - { - switch (n) { // good - case 0: // ... - case 7: // ... - } - } + 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) - // ... - } + void use2(int n) + { + if (n==0) // bad: if-then-else chain comparing against a set of constants + // ... + else if (n==7) + // ... + } ##### Enforcement @@ -7728,27 +7714,27 @@ Flag if-then-else chains that check against constants (only). ##### Example - for(int i=0; i> x; - x - } while (x<0); + int x; + do { + cin >> x; + x + } while (x<0); ##### Enforcement @@ -7867,22 +7853,22 @@ Breaking out of a nested loop. In that case, always jump forwards. ##### Example - ??? + ??? ##### Example There is a fair amount of use of the C goto-exit idiom: - void f() - { - // ... - goto exit; - // ... - goto exit; - // ... - exit: - ... common cleanup code ... - } + 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. @@ -7898,7 +7884,7 @@ This is an ad-hoc simulation of destructors. Declare your resources with handles ##### Example - ??? + ??? ##### Enforcement @@ -7912,19 +7898,19 @@ This is an ad-hoc simulation of destructors. Declare your resources with handles ##### Example - ??? + ??? ##### Note Multiple case labels of a single statement is OK: - switch (x) { - case 'a': - case 'b': - case 'f': - do_something(x); - break; - } + switch (x) { + case 'a': + case 'b': + case 'f': + do_something(x); + break; + } ##### Enforcement @@ -7938,7 +7924,7 @@ Multiple case labels of a single statement is OK: ##### Example - ??? + ??? ##### Enforcement @@ -7952,12 +7938,12 @@ Multiple case labels of a single statement is OK: ##### Example - for (i=0; i