From 2a9d0a43b8cbc0c49bfaa3a454c95f0f0d32f299 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Wed, 10 Aug 2016 22:59:00 +0200 Subject: [PATCH 01/22] dodgy example code --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 1c92c21..654f5d9 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -7099,7 +7099,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. @@ -7112,7 +7112,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 From 005e546d634eb15d99096d8a0e8e735a7c7df32e Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Wed, 10 Aug 2016 23:00:07 +0200 Subject: [PATCH 02/22] bad return type --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 654f5d9..71c2d32 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -4532,7 +4532,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 // ... }; From 165c61e7ce506d8651be77c6bf56c7a91caccfe2 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 10:27:03 +0200 Subject: [PATCH 03/22] bad signature # Conflicts: # CppCoreGuidelines.md --- CppCoreGuidelines.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 71c2d32..3f804e4 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -11452,12 +11452,12 @@ Defining "small amount" precisely is impossible. ##### Example string modify1(string); - void modify2(shared_ptr); void fct(string& s) { - auto res = async(modify1,string); - async(modify2,&s); + auto res = async(modify1, s); + async(modify2, &s); } The call of `modify1` involves copying two `string` values; the call of `modify2` does not. From bf11606c1a81071c1ba004ada1acd924cb9ad758 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 10:19:00 +0200 Subject: [PATCH 04/22] completely implement assignOperator --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 3f804e4..196a722 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -5476,9 +5476,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. From 01b66d9a7e537ac2e17fb4569ce03b02501534b2 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 10:51:58 +0200 Subject: [PATCH 05/22] unique variable names in example --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 196a722..42ad27d 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -16181,7 +16181,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; }; @@ -16194,7 +16194,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); From dc683521eed5981b783a2d379cfbfaea5e585285 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 17:18:50 +0200 Subject: [PATCH 06/22] wrong sample class --- CppCoreGuidelines.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 42ad27d..0e20c42 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -4169,7 +4169,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 } @@ -4440,8 +4440,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). From 0b15a43c54eed6e64a76144176d95693c8858989 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 17:00:08 +0200 Subject: [PATCH 07/22] missing comma --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 0e20c42..aa1f0df 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -4945,7 +4945,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 +4964,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) From b9f41b5cd072c7d14e4d66a75367928d017ace24 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 17:05:03 +0200 Subject: [PATCH 08/22] missing template argument --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index aa1f0df..edaa9cb 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -4619,7 +4619,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 From 099748957349614f9a35f055581ebd4f13f920a6 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 17:18:38 +0200 Subject: [PATCH 09/22] bad signature --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index edaa9cb..d693b6c 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -4279,7 +4279,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 { From 04345df6d0a750ff7eb10ad4d6419bc66980e62d Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 17:50:12 +0200 Subject: [PATCH 10/22] remove duplicate constructor --- CppCoreGuidelines.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index d693b6c..520f427 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 From 6a8728a05451a23e5aa514f32ea4817c0b0a5a3e Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 17:53:18 +0200 Subject: [PATCH 11/22] qualified access --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 520f427..6a506d1 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -4541,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(); // ... } From 12bdb63b06da25a939d3a9fcd109985c2ce576ac Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 17:56:53 +0200 Subject: [PATCH 12/22] fix parens --- CppCoreGuidelines.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 6a506d1..7b0c207 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -11527,8 +11527,8 @@ Thread creation is expensive. void master(istream& is) { - for (Message m; is>>m; ) - run_list.push_back(new thread(worker,m);} + for (Message m; is >> m; ) + run_list.push_back(new thread(worker, m)); } This spawns a `thread` per message, and the `run_list` is presumably managed to destroy those tasks once they are finished. @@ -11913,9 +11913,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); } From 4e46bd9a8b77c2b492711142fbe0fdeedba09d3f Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 18:13:29 +0200 Subject: [PATCH 13/22] fix indent --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 7b0c207..c50f851 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -11104,7 +11104,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() { @@ -12127,7 +12127,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 { From 46a26976babc0584c20f2e228085e8755127dd36 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 18:17:25 +0200 Subject: [PATCH 14/22] typo in method invocation --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index c50f851..34e965d 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -11353,7 +11353,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? ... } From bcca1488e8b799b3b52f57d6cdd6a054bd5f6132 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 18:32:11 +0200 Subject: [PATCH 15/22] unique var names in example --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 34e965d..4f22a79 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -13044,8 +13044,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 From 3e1519beb3441ec9d2b2da4b3091e1ecf2e204e4 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 18:50:53 +0200 Subject: [PATCH 16/22] bad parens within name --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 4f22a79..6f73058 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -12377,7 +12377,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); // ... From d6ffbfdcc2c2f3ea051a92020bb918fcda93cc11 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 18:44:49 +0200 Subject: [PATCH 17/22] missing semicolon --- CppCoreGuidelines.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 6f73058..0dadb30 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -12171,7 +12171,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}{} vctor(int s) : elem{new double},sz{s} { /* initialize elements */ } @@ -12182,7 +12182,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. @@ -12360,7 +12360,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 } From 9d4fc0b5cb188392e1505e5fc098b6d7166a7065 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 19:00:23 +0200 Subject: [PATCH 18/22] bad type --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 0dadb30..d81d8f8 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -12284,7 +12284,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 } @@ -12721,7 +12721,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) From b14fe453a4233ea8e9709004f264c07a4046113a Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 19:04:23 +0200 Subject: [PATCH 19/22] avoid keyword name --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index d81d8f8..5c87b20 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -7449,7 +7449,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. @@ -7459,7 +7459,7 @@ Use `constexpr` values instead. For example: constexpr int red = 0x,FF0000; constexpr short scale = 4; - constexpr bool signed = true; + constexpr bool is_signed = true; ##### Enforcement From 9160dbb818f69ab91948d33f57a31f4c5dca77d3 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 19:07:44 +0200 Subject: [PATCH 20/22] invalid enums --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index 5c87b20..fcc9ce8 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -7512,8 +7512,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 From 6867d13363cf95937596c72831868c73e996019e Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 19:07:51 +0200 Subject: [PATCH 21/22] type error --- CppCoreGuidelines.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index fcc9ce8..cf65aa6 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -8526,7 +8526,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; } From ced78ffad102487415af540edcc7643dd7883ca1 Mon Sep 17 00:00:00 2001 From: Thibault Kruse Date: Thu, 11 Aug 2016 19:52:06 +0200 Subject: [PATCH 22/22] typo --- CppCoreGuidelines.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CppCoreGuidelines.md b/CppCoreGuidelines.md index cf65aa6..7db7bc1 100644 --- a/CppCoreGuidelines.md +++ b/CppCoreGuidelines.md @@ -6225,7 +6225,7 @@ Now `Shape` is a poor example of a class with an implementation, but bare with us because this is just a simple example of a technique aimed at more complex hierarchies. class Impl::Circle : public Circle, public Impl::Shape { // implementation - publc: + public: // constructors, destructor int radius() { /* ... */ } @@ -6240,7 +6240,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 // ... }