StarMirror/CppCoreGuidelines
1
0
Fork 0
mirror of https://github.com/isocpp/CppCoreGuidelines.git synced 2024-03-22 13:30:58 +08:00
Code Issues Projects Releases Wiki Activity

Merge pull request #666 from tkruse/dodgy-code

Browse Source 
Minor dodgy code issues
This commit is contained in:
Gabriel Dos Reis 2016-08-22 11:33:01 -07:00 committed by GitHub
commit 4efa0fb291
1 changed files with 35 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

71
CppCoreGuidelines.md
View File

@ -3524,7 +3524,6 @@ but:
int y; int y;
Month m; Month m;
char d; // day char d; // day
Date(int yy, Month mm, char dd);
}; };
##### Note ##### 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! ~Smart_ptr2() { delete p; } // p is an owner!
}; };
void use(Smart_ptr<int> p1) void use(Smart_ptr2<int> p1)
{ {
auto p2 = p1; // error: double deletion auto p2 = p1; // error: double deletion
} }
@ -4279,7 +4278,7 @@ See [this in the Discussion section](#Sd-dtor).
##### Example, bad ##### Example, bad
struct Base { // BAD: no virtual destructor struct Base { // BAD: no virtual destructor
virtual f(); virtual void f();
}; };
struct D : Base { 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 Rec2(const string& ss, int ii = 0) :s{ss}, i{ii} {} // redundant
}; };
Rec r1 {"Foo", 7}; Rec2 r1 {"Foo", 7};
Rec r2 {"Bar"}; Rec2 r2 {"Bar"};
The `Rec2` constructor is redundant. The `Rec2` constructor is redundant.
Also, the default for `int` would be better done as a [member initializer](#Rc-in-class-initializer). 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 void read(); // read from f
// ... // ...
}; };
@ -4542,7 +4541,7 @@ Leaving behind an invalid object is asking for trouble.
X3 file {"Heraclides"}; X3 file {"Heraclides"};
file.read(); // crash or bad read! file.read(); // crash or bad read!
// ... // ...
if (is_valid()) { if (file.is_valid()) {
file.read(); file.read();
// ... // ...
} }
@ -4619,7 +4618,7 @@ A class with members that all have default constructors implicitly gets a defaul
struct X { struct X {
string s; string s;
vector v; vector<int> v;
}; };
X x; // means X{{}, {}}; that is the empty string and the empty vector 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; int y;
public: public:
Date(int ii, Month mm, year yy) 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{}; } { if (!valid(i, m, y)) throw Bad_date{}; }
Date(int ii, Month mm) Date(int ii, Month mm)
@ -4964,7 +4963,7 @@ The common action gets tedious to write and may accidentally not be common.
int y; int y;
public: public:
Date2(int ii, Month mm, year yy) 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{}; } { if (!valid(i, m, y)) throw Bad_date{}; }
Date2(int ii, Month mm) 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() { cerr << "exiting " << message << '\n'; }
Tracer2(const Tracer2& a) : message{a.message} {} 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(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. 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 class Impl::Circle : public Circle, public Impl::Shape { // implementation
publc: public:
// constructors, destructor // constructors, destructor
int radius() { /* ... */ } 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 class Impl::Smiley : Public Smiley, public Impl::Circle { // implementation
publc: public:
// constructors, destructor // 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 bool operator==(S, S); // OK: in the same namespace as S, and even next to S
S s; S s;
bool s == s; bool x = (s == s);
This is what a default `==` would do, if we had such defaults. 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; N::S s;
bool s == s; // finds N::operator==() by ADL bool x = (s == s); // finds N::operator==() by ADL
##### Example, bad ##### Example, bad
@ -7456,7 +7455,7 @@ If you can't name an enumeration, the values are not related
##### Example, bad ##### 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. 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 int red = 0xFF0000;
constexpr short scale = 4; constexpr short scale = 4;
constexpr bool signed = true; constexpr bool is_signed = true;
##### Enforcement ##### Enforcement
@ -7519,8 +7518,8 @@ The default gives a consecutive set of values that is good for `switch`-statemen
##### Example ##### Example
enum class Col1 { red, yellow, blue }; enum class Col1 { red, yellow, blue };
enum class Col2 { red = 1, red = 2, blue = 2 }; // typo enum class Col2 { red = 1, yellow = 2, blue = 2 }; // typo
enum class Month { jan = 1, feb, mar, apr, mar, jun, enum class Month { jan = 1, feb, mar, apr, may, jun,
jul, august, sep, oct, nov, dec }; // starting with 1 is conventional 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 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); is.read(s, maxstring);
res[elemcount++] = s; res[elemcount++] = s;
} }
nread = elemcount; nread = &elemcount;
return res; 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. 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() 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) void use(int n)
{ {
thread t { thricky, this, n }; thread t { tricky, this, n };
// ... // ...
// ... should I join here? ... // ... should I join here? ...
} }
@ -11923,9 +11922,9 @@ Double-checked locking is easy to mess up.
atomic<bool> x_init; atomic<bool> x_init;
if (!x_init.load(memory_order_acquire) { if (!x_init.load(memory_order_acquire)) {
lock_guard<mutex> lck(x_mutex); lock_guard<mutex> lck(x_mutex);
if (!x_init.load(memory_order_relaxed) { if (!x_init.load(memory_order_relaxed)) {
// ... initialize x ... // ... initialize x ...
x_init.store(true, memory_order_release); 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 ##### Example, don't
// don't: exception not used for error handling // don't: exception not used for error handling
int find_index(vector<string>& vec, const string& x) int find_index(vector<string>& vec, const string& x)
{ {
try { try {
@ -12181,7 +12180,7 @@ Not all member functions can be called.
##### Example ##### Example
class vector { // very simplified vector of doubles 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: public:
vector() : elem{nullptr}, sz{0}{} vector() : elem{nullptr}, sz{0}{}
vector(int s) : elem{new double}, sz{s} { /* initialize elements */ } vector(int s) : elem{new double}, sz{s} { /* initialize elements */ }
@ -12192,7 +12191,7 @@ Not all member functions can be called.
private: private:
owner<double*> elem; owner<double*> elem;
int sz; int sz;
} };
The class invariant - here stated as a comment - is established by the constructors. The class invariant - here stated as a comment - is established by the constructors.
`new` throws if it cannot allocate the required memory. `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 // handle error or exit
} }
Ifstream fs("foo"); // not std::ifstream: valid() added ifstream fs("foo"); // not std::ifstream: valid() added
if (!fs.valid()) { if (!fs.valid()) {
// handle error or exit // handle error or exit
} }
@ -12370,7 +12369,7 @@ That would be a leak.
void leak(int x) // don't: may leak void leak(int x) // don't: may leak
{ {
auto p = new int{7}; 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 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: 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<int> v(7); vector<int> 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 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) 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 ##### Example
double x = f(2); // possible run-time evaluation double x = f(2); // possible run-time evaluation
const double x = f(2); // possible run-time evaluation const double y = f(2); // possible run-time evaluation
constexpr double y = f(2); // error unless f(2) can be evaluated at compile time constexpr double z = f(2); // error unless f(2) can be evaluated at compile time
##### Note ##### Note
@ -16189,7 +16188,7 @@ Note that a C-style `(T)expression` cast means to perform the first of the follo
##### Example, bad ##### Example, bad
std::string s = "hello world"; std::string s = "hello world";
double* p = (double*)(&s); // BAD double* p0 = (double*)(&s); // BAD
class base { public: virtual ~base() = 0; }; 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; 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 // BAD, tries to treat d1 as a derived2, which it is not
derived2* p2 = (derived2*)(p); derived2* p2 = (derived2*)(p);