diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 6829ac8..8cd763f 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -3524,7 +3524,6 @@ but: int y; Month m; char d; // day - Date(int yy, Month mm, char dd); }; ##### Note @@ -4169,7 +4168,7 @@ Note that if you define a destructor, you must define or delete [all default ope ~Smart_ptr2() { delete p; } // p is an owner! }; - void use(Smart_ptr p1) + void use(Smart_ptr2 p1) { auto p2 = p1; // error: double deletion } @@ -4279,7 +4278,7 @@ See [this in the Discussion section](#Sd-dtor). ##### Example, bad struct Base { // BAD: no virtual destructor - virtual f(); + virtual void f(); }; struct D : Base { @@ -4440,8 +4439,8 @@ The C++11 initializer list rule eliminates the need for many constructors. For e Rec2(const string& ss, int ii = 0) :s{ss}, i{ii} {} // redundant }; - Rec r1 {"Foo", 7}; - Rec r2 {"Bar"}; + Rec2 r1 {"Foo", 7}; + Rec2 r2 {"Bar"}; The `Rec2` constructor is redundant. Also, the default for `int` would be better done as a [member initializer](#Rc-in-class-initializer). @@ -4532,7 +4531,7 @@ Leaving behind an invalid object is asking for trouble. // ... } - void is_valid() { return valid; } + bool is_valid() { return valid; } void read(); // read from f // ... }; @@ -4542,7 +4541,7 @@ Leaving behind an invalid object is asking for trouble. X3 file {"Heraclides"}; file.read(); // crash or bad read! // ... - if (is_valid()) { + if (file.is_valid()) { file.read(); // ... } @@ -4619,7 +4618,7 @@ A class with members that all have default constructors implicitly gets a defaul struct X { string s; - vector v; + vector v; }; X x; // means X{{}, {}}; that is the empty string and the empty vector @@ -4945,7 +4944,7 @@ To avoid repetition and accidental differences. int y; public: Date(int ii, Month mm, year yy) - :i{ii}, m{mm} y{yy} + :i{ii}, m{mm}, y{yy} { if (!valid(i, m, y)) throw Bad_date{}; } Date(int ii, Month mm) @@ -4964,7 +4963,7 @@ The common action gets tedious to write and may accidentally not be common. int y; public: Date2(int ii, Month mm, year yy) - :i{ii}, m{mm} y{yy} + :i{ii}, m{mm}, y{yy} { if (!valid(i, m, y)) throw Bad_date{}; } Date2(int ii, Month mm) @@ -5476,9 +5475,9 @@ Because we defined the destructor, we must define the copy and move operations. ~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; return *this; } Tracer2(Tracer2&& a) :message{a.message} {} - Tracer2& operator=(Tracer2&& a) { message = a.message; } + Tracer2& operator=(Tracer2&& a) { message = a.message; return *this; } }; Writing out the bodies of the copy and move operations is verbose, tedious, and error-prone. A compiler does it better. @@ -6231,7 +6230,7 @@ but bear with us because this is just a simple example of a technique aimed at m class Impl::Circle : public Circle, public Impl::Shape { // implementation - publc: + public: // constructors, destructor int radius() { /* ... */ } @@ -6246,7 +6245,7 @@ And we could extend the hierarchies by adding a Smiley class (:-)): }; class Impl::Smiley : Public Smiley, public Impl::Circle { // implementation - publc: + public: // constructors, destructor // ... } @@ -7105,7 +7104,7 @@ Avoiding inconsistent definition in different namespaces bool operator==(S, S); // OK: in the same namespace as S, and even next to S S s; - bool s == s; + bool x = (s == s); This is what a default `==` would do, if we had such defaults. @@ -7118,7 +7117,7 @@ This is what a default `==` would do, if we had such defaults. N::S s; - bool s == s; // finds N::operator==() by ADL + bool x = (s == s); // finds N::operator==() by ADL ##### Example, bad @@ -7456,7 +7455,7 @@ If you can't name an enumeration, the values are not related ##### Example, bad - enum { red = 0xFF0000, scale = 4, signed = 1 }; + enum { red = 0xFF0000, scale = 4, is_signed = 1 }; Such code is not uncommon in code written before there were convenient alternative ways of specifying integer constants. @@ -7466,7 +7465,7 @@ Use `constexpr` values instead. For example: constexpr int red = 0xFF0000; constexpr short scale = 4; - constexpr bool signed = true; + constexpr bool is_signed = true; ##### Enforcement @@ -7519,8 +7518,8 @@ The default gives a consecutive set of values that is good for `switch`-statemen ##### Example enum class Col1 { red, yellow, blue }; - enum class Col2 { red = 1, red = 2, blue = 2 }; // typo - enum class Month { jan = 1, feb, mar, apr, mar, jun, + enum class Col2 { red = 1, yellow = 2, blue = 2 }; // typo + enum class Month { jan = 1, feb, mar, apr, may, jun, jul, august, sep, oct, nov, dec }; // starting with 1 is conventional enum class Base_flag { dec = 1, oct = dec << 1, hex = dec << 2 }; // set of bits @@ -8533,7 +8532,7 @@ The more traditional and lower-level near-equivalent is longer, messier, harder is.read(s, maxstring); res[elemcount++] = s; } - nread = elemcount; + nread = &elemcount; return res; } @@ -11114,7 +11113,7 @@ Avoids nasty errors from unreleased locks. Sooner or later, someone will forget the `mtx.unlock()`, place a `return` in the `... do stuff ...`, throw an exception, or something. - mutex mtx; + mutex mtx; void do_stuff() { @@ -11363,7 +11362,7 @@ The plain `thread`s should be assumed to use the full generality of `std::thread void use(int n) { - thread t { thricky, this, n }; + thread t { tricky, this, n }; // ... // ... should I join here? ... } @@ -11923,9 +11922,9 @@ Double-checked locking is easy to mess up. atomic x_init; - if (!x_init.load(memory_order_acquire) { + if (!x_init.load(memory_order_acquire)) { lock_guard lck(x_mutex); - if (!x_init.load(memory_order_relaxed) { + if (!x_init.load(memory_order_relaxed)) { // ... initialize x ... x_init.store(true, memory_order_release); } @@ -12137,7 +12136,7 @@ C++ implementations tend to be optimized based on the assumption that exceptions ##### Example, don't - // don't: exception not used for error handling + // don't: exception not used for error handling int find_index(vector& vec, const string& x) { try { @@ -12181,7 +12180,7 @@ Not all member functions can be called. ##### Example class vector { // very simplified vector of doubles - // if elem!=nullptr then elem points to sz doubles + // if elem != nullptr then elem points to sz doubles public: vector() : elem{nullptr}, sz{0}{} vector(int s) : elem{new double}, sz{s} { /* initialize elements */ } @@ -12192,7 +12191,7 @@ Not all member functions can be called. private: owner elem; int sz; - } + }; The class invariant - here stated as a comment - is established by the constructors. `new` throws if it cannot allocate the required memory. @@ -12294,7 +12293,7 @@ One strategy is to add a `valid()` operation to every resource handle: // handle error or exit } - Ifstream fs("foo"); // not std::ifstream: valid() added + ifstream fs("foo"); // not std::ifstream: valid() added if (!fs.valid()) { // handle error or exit } @@ -12370,7 +12369,7 @@ That would be a leak. void leak(int x) // don't: may leak { auto p = new int{7}; - if (x < 0) throw Get_me_out_of_here{} // may leak *p + if (x < 0) throw Get_me_out_of_here{}; // may leak *p // ... delete p; // we may never get here } @@ -12387,7 +12386,7 @@ One way of avoiding such problems is to use resource handles consistently: Another solution (often better) would be to use a local variable to eliminate explicit use of pointers: - void no_leak(_simplified(int x) + void no_leak_simplified(int x) { vector v(7); // ... @@ -12731,7 +12730,7 @@ In such cases, "crashing" is simply leaving error handling to the next level of Most programs cannot handle memory exhaustion gracefully anyway. This is roughly equivalent to - void f(Int n) + void f(int n) { // ... p = new X[n]; // throw if memory is exhausted (by default, terminate) @@ -13054,8 +13053,8 @@ Better performance, better compile-time checking, guaranteed compile-time evalua ##### Example double x = f(2); // possible run-time evaluation - const double x = f(2); // possible run-time evaluation - constexpr double y = f(2); // error unless f(2) can be evaluated at compile time + const double y = f(2); // possible run-time evaluation + constexpr double z = f(2); // error unless f(2) can be evaluated at compile time ##### Note @@ -16189,7 +16188,7 @@ Note that a C-style `(T)expression` cast means to perform the first of the follo ##### Example, bad std::string s = "hello world"; - double* p = (double*)(&s); // BAD + double* p0 = (double*)(&s); // BAD class base { public: virtual ~base() = 0; }; @@ -16202,7 +16201,7 @@ Note that a C-style `(T)expression` cast means to perform the first of the follo }; derived1 d1; - base* p = &d1; // ok, implicit conversion to pointer to base is fine + base* p1 = &d1; // ok, implicit conversion to pointer to base is fine // BAD, tries to treat d1 as a derived2, which it is not derived2* p2 = (derived2*)(p);