diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 4e079d1..dd80795 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -357,11 +357,11 @@ The second version leaves the reader guessing and opens more possibilities for u void do_something(vector& v) { string val; - cin>>val; + cin >> val; // ... int index = 0; // bad - for(int i=0; i& v) { string val; - cin>>val; + cin >> val; // ... - auto p = find(v, val); // better + auto p = find(v, val); // better // ... } @@ -439,7 +439,7 @@ Unless the intent of some code is stated (e.g., in names or comments), it is imp ##### Example int i = 0; - while (i=4); // do: compile-time check + static_assert(sizeof(Int) >= 4); // do: compile-time check int bits = 0; // don't: avoidable code - for (Int i = 1; i; i<<=1) + for (Int i = 1; i; i <<= 1) ++bits; - if (bits<32) + if (bits < 32) cerr << "Int too small\n"; // ... @@ -670,7 +670,7 @@ Avoid errors leading to (possibly unrecognized) wrong results. void increment1(int* p, int n) // bad: error prone { - 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). +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). **Reference**: `Expects()` is described in [GSL](#S-gsl). ##### Note -Prefer a formal specification of requirements, such as `Expects(p!=nullptr);` If that is infeasible, use English text in comments, such as +Prefer a formal specification of requirements, such as `Expects(p != nullptr);` If that is infeasible, use English text in comments, such as `// the sequence [p:q) is ordered using <` ##### Note @@ -1183,8 +1183,8 @@ To make it clear that the condition is a precondition and to enable tool use. int area(int height, int width) { - Expects(height>0 && width>0); // good - if (height<=0 || width<=0) my_error(); // obscure + Expects(height > 0 && width > 0); // good + if (height <= 0 || width <= 0) my_error(); // obscure // ... } @@ -1214,17 +1214,17 @@ To detect misunderstandings about the result and possibly catch erroneous implem 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. +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); + auto res = height * width; + Ensures(res > 0); return res; } @@ -1246,7 +1246,7 @@ There was no postcondition stating that the buffer should be cleared and the opt char buffer[MAX]; // ... memset(buffer, 0, MAX); - Ensures(buffer[0]==0); + Ensures(buffer[0] == 0); } ##### Note @@ -1310,7 +1310,7 @@ To make it clear that the condition is a postcondition and to enable tool use. char buffer[MAX]; // ... memset(buffer, 0, MAX); - Ensures(buffer[0]==0); + Ensures(buffer[0] == 0); } ##### Note @@ -1381,7 +1381,7 @@ However, if failing to make a connection is considered an error, then a failure int val; int error_code; tie(val, error_code) = do_something(); - if (error_code==0) { + if (error_code == 0) { // ... handle the error or exit ... } // ... use val ... @@ -1651,9 +1651,9 @@ This will force every derived class to compute a center -- even if that's non-tr 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; + virtual Point center() = 0; // pure virtual function + virtual void draw() = 0; + virtual void rotate(int) = 0; // ... // ... no data members ... }; @@ -1750,7 +1750,7 @@ If something is a well-specified action, separate it out from its surrounding co void read_and_print(istream& is) // read and print an int { int x; - if (is>>x) + if (is >> x) cout << "the int is " << x << '\n'; else cerr << "no int on input\n"; @@ -1944,9 +1944,9 @@ 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; ) + for (string s; is >> s;) res.push_back(s); return res; } @@ -2175,9 +2175,9 @@ Assuming that `Matrix` has move operations (possibly by keeping its elements in 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 @@ -2212,11 +2212,11 @@ Consider: 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`? +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 @@ -2246,7 +2246,7 @@ Clarity. Making it clear that a test for null isn't needed. void computer(not_null> p) { - if (0 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 @@ -2407,7 +2407,7 @@ A return value is harder to miss and harder to misuse than a `T&` (an in-out par struct Package { char header[16]; - char load[2024-16]; + char load[2024 - 16]; }; Package fill(); // Bad: large return value @@ -2476,11 +2476,12 @@ Using `unique_ptr` is the cheapest way to pass a pointer safely. { auto kind = read_header(is); // read header and identify the next shape on input switch (kind) { - case kCicle: + case kCircle: return make_unique(is); case kTriangle: return make_unique(is); // ... + } } ##### Note @@ -2808,7 +2809,7 @@ Functions can't capture local variables or be declared at local scope; if you ne // 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) { + for (int tasknum = 0; tasknum < max; ++tasknum) { pool.run([=, &v]{ /* ... @@ -3082,7 +3083,7 @@ Flag non-`const` member functions that do not write to their objects One ideal for a class is to be a regular type. That means roughly "behaves like an `int`." A concrete type is the simplest kind of class. A value of regular type can be copied and the result of a copy is an independent object with the same value as the original. -If a concrete type has both `=` and `==`, `a=b` should result in `a==b` being `true`. +If a concrete type has both `=` and `==`, `a=b` should result in `a == b` being `true`. Concrete classes without assignment and equality can be defined, but they are (and should be) rare. The C++ built-in types are regular, and so are standard-library classes, such as `string`, `vector`, and `map`. Concrete types are also often referred to as value types to distinguish them from types uses as part of a hierarchy. @@ -3156,15 +3157,15 @@ Regular types are easier to understand and reason about than types that are not 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!"); + if (!(b1 == b2)) error("impossible!"); b2.name = "the other bundle"; - if (b1==b2) error("No!"); + 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`. +In particular, if a concrete type has an assignment also give it an equals operator so that `a=b` implies `a == b`. ##### Enforcement @@ -3391,7 +3392,7 @@ Only define a non-default destructor if a class needs to execute code that is no void test() { - auto act = finally([]{ cout<<"Exit test\n"; }); // establish exit action + auto act = finally([]{ cout << "Exit test\n"; }); // establish exit action // ... if (something) return; // act done here // ... @@ -3411,7 +3412,7 @@ There are two general categories of classes that need a user-defined destructor: class Foo { // bad; use the default destructor public: // ... - ~Foo() { s=""; i=0; vi.clear(); } // clean up + ~Foo() { s = ""; i = 0; vi.clear(); } // clean up private: string s; int i; @@ -3859,7 +3860,7 @@ Leaving behind an invalid object is asking for trouble. X2(const string& name) :f{fopen(name.c_str(), "r")} { - if (f==nullptr) throw runtime_error{"could not open" + name}; + if (f == nullptr) throw runtime_error{"could not open" + name}; // ... } @@ -3884,7 +3885,7 @@ Leaving behind an invalid object is asking for trouble. X3(const string& name) :f{fopen(name.c_str(), "r")}, valid{false} { - if (f) valid=true; + if (f) valid = true; // ... } @@ -3958,7 +3959,7 @@ Being able to set a value to "the default" without operations that might fail si 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} {} + Vector0(int n) :elem{new T[n]}, space{elem + n}, last{elem} {} // ... private: own elem; @@ -3976,7 +3977,7 @@ For example, `Vector0 v(100)` costs 100 allocations. 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} {} + Vector1(int n) :elem{new T[n]}, space{elem + n}, last{elem} {} // ... private: own elem = nullptr; @@ -4073,7 +4074,7 @@ To minimize confusion and errors. That is the order in which the initialization // ... }; - Foo x(1); // surprise: x.m1==x.m2==2 + Foo x(1); // surprise: x.m1 == x.m2 == 2 ##### Enforcement @@ -4383,7 +4384,7 @@ See [copy constructor vs. `clone()`](#Rc-copy-virtual). ##### Reason -That is the generally assumed semantics. After `x=y`, we should have `x==y`. +That is the generally assumed semantics. After `x=y`, we should have `x == y`. After a copy `x` and `y` can be independent objects (value semantics, the way non-pointer built-in types and the standard-library types work) or refer to a shared object (pointer semantics, the way pointers work). ##### Example @@ -4402,20 +4403,20 @@ After a copy `x` and `y` can be independent objects (value semantics, the way no bool operator==(const X& a, const X& b) { - return sz==a.sz && equal(p, p+sz, a.p, a.p+sz); + return sz == a.sz && equal(p, p + sz, a.p, a.p + sz); } X::X(const X& a) :p{new T}, sz{a.sz} { - copy(a.p, a.p+sz, a.p); + copy(a.p, a.p + sz, a.p); } X x; X y = x; - if (x!=y) throw Bad{}; + if (x != y) throw Bad{}; x.modify(); - if (x==y) throw Bad{}; // assume value semantics + if (x == y) throw Bad{}; // assume value semantics ##### Example @@ -4433,14 +4434,14 @@ After a copy `x` and `y` can be independent objects (value semantics, the way no bool operator==(const X2& a, const X2& b) { - return sz==a.sz && p==a.p; + return sz == a.sz && p == a.p; } X2 x; X2 y = x; - if (x!=y) throw Bad{}; + if (x != y) throw Bad{}; x.modify(); - if (x!=y) throw Bad{}; // assume pointer semantics + if (x != y) throw Bad{}; // assume pointer semantics ##### Note @@ -4492,7 +4493,7 @@ You can handle self-assignment by explicitly testing for self-assignment, but of Foo& Foo::operator=(const Foo& a) // OK, but there is a cost { - if (this==&a) return *this; + if (this == &a) return *this; s = a.s; i = a.i; return *this; @@ -4514,7 +4515,7 @@ Consider: ##### Enforcement -(Simple) Assignment operators should not contain the pattern `if (this==&a) return *this;` ??? +(Simple) Assignment operators should not contain the pattern `if (this == &a) return *this;` ??? ### C.63: Make move assignment non-`virtual`, take the parameter by `&&`, and return by non-`const &` @@ -4600,17 +4601,17 @@ If `x=x` changes the value of `x`, people will be surprised and bad errors may o Foo& Foo::operator=(Foo&& a) // OK, but there is a cost { - if (this==&a) return *this; // this line is redundant + 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. +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. ##### Note -There is no know general way of avoiding a `if (this==&a) return *this;` test for a move assignment and still get a correct answer (i.e., after `x=x` the value of `x` is unchanged). +There is no know general way of avoiding a `if (this == &a) return *this;` test for a move assignment and still get a correct answer (i.e., after `x=x` the value of `x` is unchanged). ##### Note @@ -4643,8 +4644,8 @@ A non-throwing move will be used more efficiently by standard-library and langua 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; } + 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; @@ -4734,8 +4735,8 @@ The compiler is more likely to get the default semantics right and you cannot im class Tracer { string message; public: - Tracer(const string& m) : message{m} { cerr << "entering " << message <<'\n'; } - ~Tracer() { cerr << "exiting " << message <<'\n'; } + Tracer(const string& m) : message{m} { cerr << "entering " << message << '\n'; } + ~Tracer() { cerr << "exiting " << message << '\n'; } Tracer(const Tracer&) = default; Tracer& operator=(const Tracer&) = default; @@ -4750,13 +4751,13 @@ Because we defined the destructor, we must define the copy and move operations. class Tracer2 { string message; public: - Tracer2(const string& m) : message{m} { cerr << "entering " << message <<'\n'; } - ~Tracer2() { cerr << "exiting " << message <<'\n'; } + 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& operator=(const Tracer2& a) { message = a.message; } Tracer2(Tracer2&& a) :message{a.message} {} - Tracer2& operator=(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. @@ -4931,14 +4932,14 @@ Asymmetric treatment of operands is surprising and a source of errors where conv 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; } + bool operator==(const B& a) const { return name == a.name && number == a.number; } // ... }; @@ -4964,7 +4965,7 @@ It is really hard to write a foolproof and useful `==` for a hierarchy. class B { string name; int number; - virtual bool operator==(const B& a) const { return name==a.name && number==a.number; } + virtual bool operator==(const B& a) const { return name == a.name && number == a.number; } // ... }; @@ -4972,18 +4973,18 @@ It is really hard to write a foolproof and useful `==` for a hierarchy. class D :B { char character; - virtual bool operator==(const D& a) const { return name==a.name && number==a.number && character==a.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 + b == d; // compares name and number, ignores d's character + d == b; // error: no == defined D d2; - d==d2; // compares name, number, and character + d == d2; // compares name, number, and character B& b2 = d2; - b2==d; // compares name and number, ignores d2's and d's character + 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 @@ -5575,7 +5576,7 @@ Avoid resource leaks. { 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 p {new{7}); // OK: but repetitive + unique_ptr p {new{7}}; // OK: but repetitive auto q = make_unique(7); // Better: no repetition of Foo @@ -5610,7 +5611,7 @@ 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 @@ -5665,7 +5666,7 @@ Minimize surprises. ##### 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) @@ -5678,11 +5679,11 @@ Possibly impossible. ##### Reason If you use member functions, you need two. -Unless you use a non-member function for (say) `==`, `a==b` and `b==a` will be subtly different. +Unless you use a non-member function for (say) `==`, `a == b` and `b == a` will be subtly different. ##### 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 @@ -5768,7 +5769,7 @@ just to gain a minor convenience. void user(zstring p) { - if (*p=="") { + if (*p == "") { String s {"Trouble ahead!"}; // ... p = s; @@ -6353,7 +6354,7 @@ If you don't, an exception or a return may lead to a leak. { FILE* f = fopen(name, "r"); // open the file vector buf(1024); - auto _ = finally([] { fclose(f); } // remember to close the file + auto _ = finally([] { fclose(f); }) // remember to close the file // ... } @@ -6897,8 +6898,8 @@ 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; ) + for (string s; is >> s;) res.push_back(s); return res; } @@ -6928,7 +6929,7 @@ The more traditional and lower-level near-equivalent is longer, messier, harder { auto res = new char*[maxelem]; int elemcount = 0; - while (is && elemcount(ps)) { // good: pc is local to if-statement // ...deal with Circle ... @@ -7019,10 +7020,10 @@ Readability. Minimize resource retention. void use() { - for (string s; cin>>s; ) + 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 //* ... } @@ -7052,7 +7053,7 @@ Conventional short, local names increase readability: template // good void print(ostream& os, const vector& v) { - for (int i = 0; i& current_vector) { for (int current_element_index = 0; - current_element_index>s; // s expands to hold the string + 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; + 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; + cin >> i; A good optimizer should know about input operations and eliminate the redundant operation. @@ -7483,7 +7484,7 @@ Using `std::unique_ptr` is the simplest way to avoid leaks. And it is free compa // ... } -If `leak==true` the object pointer to by `p2` is leaked and the object pointed to by `p1` is not. +If `leak == true` the object pointer to by `p2` is leaked and the object pointed to by `p1` is not. ##### Enforcement @@ -7499,7 +7500,7 @@ That way you can't change the value by mistake. That way may offer the compiler void f(int n) { - const int bufmax = 2*n+2; // good: we can't change bufmax by accident + const int bufmax = 2 * n + 2; // good: we can't change bufmax by accident int xmax = n; // suspicious: is xmax intended to change? // ... } @@ -7519,8 +7520,8 @@ Readability. void use() { int i; - for (i=0; i<20; ++i) { /* ... */ } - for (i=0; i<200; ++i) { /* ... */ } // bad: i recycled + for (i = 0; i < 20; ++i) { /* ... */ } + for (i = 0; i < 200; ++i) { /* ... */ } // bad: i recycled } ##### Enforcement @@ -7580,7 +7581,7 @@ It nicely encapsulates local initialization, including cleaning up scratch varia ##### Example, bad widget x; // should be const, but: - for(auto i=2; i <= N; ++i) { // this could be some + 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 @@ -7589,7 +7590,7 @@ It nicely encapsulates local initialization, including cleaning up scratch varia const widget x = [&]{ widget val; // assume that widget has a default constructor - for(auto i=2; i <= N; ++i) { // this could be some + 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; @@ -7600,7 +7601,7 @@ It nicely encapsulates local initialization, including cleaning up scratch varia string var = [&]{ if (!in) return ""; // default string s; - for (char c : in>>c) + for (char c : in >> c) s += toupper(c); return s; }(); // note () @@ -7729,9 +7730,9 @@ rather than: void use2(int n) { - if (n==0) // bad: if-then-else chain comparing against a set of constants + if (n == 0) // bad: if-then-else chain comparing against a set of constants // ... - else if (n==7) + else if (n == 7) // ... } @@ -7747,23 +7748,23 @@ Readability. Error prevention. Efficiency. ##### Example - for(int i=0; i> x; x - } while (x<0); + } while (x < 0); ##### Enforcement @@ -7971,7 +7972,7 @@ Readability. ##### Example - for (i=0; i