mirror of
https://github.com/isocpp/CppCoreGuidelines.git
synced 2024-03-22 13:30:58 +08:00
Shorten Fix longest C++ lines longer than 100 chars
This commit is contained in:
parent
111dcfada1
commit
da92068a5f
|
@ -600,7 +600,8 @@ Ideally we catch all errors (that are not errors in the programmer's logic) at e
|
||||||
|
|
||||||
void g(int n)
|
void g(int n)
|
||||||
{
|
{
|
||||||
f(new int[n]); // bad: the number of elements is not passed to f()
|
// bad: the number of elements is not passed to f()
|
||||||
|
f(new int[n]);
|
||||||
}
|
}
|
||||||
|
|
||||||
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.
|
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.
|
||||||
|
@ -625,9 +626,10 @@ 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:
|
The standard library resource management pointers fail to pass the size when they point to an object:
|
||||||
|
|
||||||
extern void f3(unique_ptr<int[]>, int n); // separately compiled, possibly dynamically loaded
|
// separately compiled, possibly dynamically loaded
|
||||||
// NB: this assumes the calling code is ABI-compatible, using a
|
// NB: this assumes the calling code is A BI-compatible, using a
|
||||||
// compatible C++ compiler and the same stdlib implementation
|
// compatible C++ compiler and the same stdlib implementation
|
||||||
|
extern void f3(unique_ptr<int[]>, int n);
|
||||||
|
|
||||||
void g3(int n)
|
void g3(int n)
|
||||||
{
|
{
|
||||||
|
@ -1565,8 +1567,9 @@ 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*`.
|
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<zstring> p); // we can assume that p cannot be nullptr
|
// we can assume that p cannot be nullptr
|
||||||
// we can assume that p points to a zero-terminated array of characters
|
// we can assume that p points to a zero-terminated array of characters
|
||||||
|
int length(not_null<zstring> p);
|
||||||
|
|
||||||
Note: `length()` is, of course, `std::strlen()` in disguise.
|
Note: `length()` is, of course, `std::strlen()` in disguise.
|
||||||
|
|
||||||
|
@ -2237,10 +2240,11 @@ Passing a shared smart pointer (e.g., `std::shared_ptr`) implies a run-time cost
|
||||||
##### Example
|
##### Example
|
||||||
|
|
||||||
void f(int*); // accepts any int*
|
void f(int*); // accepts any int*
|
||||||
void g(unique_ptr<int>); // can only accept ints for which you want to transfer ownership
|
void g(unique_ptr<int>); // accepts ints to transfer ownership
|
||||||
void g(shared_ptr<int>); // can only accept ints for which you are willing to share ownership
|
void g(shared_ptr<int>); // accepts ints to share ownership
|
||||||
|
|
||||||
void h(const unique_ptr<int>&); // doesn’t change ownership, but requires a particular ownership of the caller.
|
// doesn’t change ownership, but requires a particular ownership of the caller
|
||||||
|
void h(const unique_ptr<int>&);
|
||||||
|
|
||||||
void h(int&); // accepts any int
|
void h(int&); // accepts any int
|
||||||
|
|
||||||
|
@ -2473,9 +2477,11 @@ If you have multiple values to return, [use a tuple](#Rf-out-multi) or similar m
|
||||||
|
|
||||||
##### Example
|
##### Example
|
||||||
|
|
||||||
vector<const int*> find_all(const vector<int>&, int x); // OK: return pointers to elements with the value x
|
// OK: return pointers to elements with the value x
|
||||||
|
vector<const int*> find_all(const vector<int>&, int x);
|
||||||
|
|
||||||
void find_all(const vector<int>&, vector<const int*>& out, int x); // Bad: place pointers to elements with value x in out
|
// Bad: place pointers to elements with value x in out
|
||||||
|
void find_all(const vector<int>&, vector<const int*>& out, int x);
|
||||||
|
|
||||||
##### Note
|
##### Note
|
||||||
|
|
||||||
|
@ -2518,14 +2524,16 @@ And yes, C++ does have multiple return values, by convention of using a `tuple`,
|
||||||
|
|
||||||
##### Example
|
##### Example
|
||||||
|
|
||||||
int f(const string& input, /*output only*/ string& output_data) // BAD: output-only parameter documented in a comment
|
// BAD: output-only parameter documented in a comment
|
||||||
|
int f(const string& input, /*output only*/ string& output_data)
|
||||||
{
|
{
|
||||||
// ...
|
// ...
|
||||||
output_data = something();
|
output_data = something();
|
||||||
return status;
|
return status;
|
||||||
}
|
}
|
||||||
|
|
||||||
tuple<int, string> f(const string& input) // GOOD: self-documenting
|
// GOOD: self-documenting
|
||||||
|
tuple<int, string> f(const string& input)
|
||||||
{
|
{
|
||||||
// ...
|
// ...
|
||||||
return make_tuple(status, something());
|
return make_tuple(status, something());
|
||||||
|
@ -2583,9 +2591,17 @@ In traditional C and C++ code, plain `T*` is used for many weakly-related purpos
|
||||||
|
|
||||||
void use(int* p, char* s, int* q)
|
void use(int* p, char* s, int* q)
|
||||||
{
|
{
|
||||||
*++p = 666; // Bad: we don't know if p points to two elements; assume it does not or use span<int>
|
// Bad: we don't know if p points to two elements; assume it does not or
|
||||||
cout << s; // Bad: we don't know if that s points to a zero-terminated array of char; assume it does not or use zstring
|
// use span<int>
|
||||||
delete q; // Bad: we don't know if *q is allocated on the free store; assume it does not or use owner
|
*++p = 666;
|
||||||
|
|
||||||
|
// Bad: we don't know if that s points to a zero-terminated array of char;
|
||||||
|
// assume it does not or use zstring
|
||||||
|
cout << s;
|
||||||
|
|
||||||
|
// Bad: we don't know if *q is allocated on the free store; assume it does
|
||||||
|
// not or use owner
|
||||||
|
delete q;
|
||||||
}
|
}
|
||||||
|
|
||||||
##### Note
|
##### Note
|
||||||
|
@ -2618,9 +2634,11 @@ Consider:
|
||||||
|
|
||||||
When I call `length(p)` should I test for `p == nullptr` first? Should the implementation of `length()` test for `p == nullptr`?
|
When I call `length(p)` should I test for `p == nullptr` first? Should the implementation of `length()` test for `p == nullptr`?
|
||||||
|
|
||||||
int length(not_null<Record*> p); // it is the caller's job to make sure p != nullptr
|
// it is the caller's job to make sure p != nullptr
|
||||||
|
int length(not_null<Record*> p);
|
||||||
|
|
||||||
int length(Record* p); // the implementor of length() must assume that p == nullptr is possible
|
// the implementor of length() must assume that p == nullptr is possible
|
||||||
|
int length(Record* p);
|
||||||
|
|
||||||
##### Note
|
##### Note
|
||||||
|
|
||||||
|
@ -2663,10 +2681,14 @@ A `span` represents a range of elements, but how do we manipulate elements of th
|
||||||
|
|
||||||
void f(span<int> s)
|
void f(span<int> s)
|
||||||
{
|
{
|
||||||
for (int x : s) cout << x << '\n'; // range traversal (guaranteed correct)
|
// range traversal (guaranteed correct)
|
||||||
for (int i = 0; i < s.size(); ++i) cout << x << '\n'; // C-style traversal (potentially checked)
|
for (int x : s) cout << x << '\n';
|
||||||
s[7] = 9; // random access (potentially checked)
|
// C-style traversal (potentially checked)
|
||||||
std::sort(&s[0], &s[s.size() / 2]); // extract pointers (potentially checked)
|
for (int i = 0; i < s.size(); ++i) cout << x << '\n';
|
||||||
|
// random access (potentially checked)
|
||||||
|
s[7] = 9;
|
||||||
|
// extract pointers (potentially checked)
|
||||||
|
std::sort(&s[0], &s[s.size() / 2]);
|
||||||
}
|
}
|
||||||
|
|
||||||
##### Note
|
##### Note
|
||||||
|
@ -2696,9 +2718,11 @@ Consider:
|
||||||
|
|
||||||
When I call `length(s)` should I test for `s == nullptr` first? Should the implementation of `length()` test for `p == nullptr`?
|
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
|
// the implementor of length() must assume that p == nullptr is possible
|
||||||
|
int length(zstring p);
|
||||||
|
|
||||||
int length(not_null<zstring> p); // it is the caller's job to make sure p != nullptr
|
// it is the caller's job to make sure p != nullptr
|
||||||
|
int length(not_null<zstring> p);
|
||||||
|
|
||||||
##### Note
|
##### Note
|
||||||
|
|
||||||
|
@ -3175,19 +3199,22 @@ Pointers and references to locals shouldn't outlive their scope. Lambdas that ca
|
||||||
|
|
||||||
{
|
{
|
||||||
int local = 42;
|
int local = 42;
|
||||||
thread_pool.queue_work([&]{ process(local); }); // Want a reference to local.
|
|
||||||
|
// Want a reference to local.
|
||||||
// Note, that after program exits this scope,
|
// Note, that after program exits this scope,
|
||||||
// local no longer exists, therefore
|
// local no longer exists, therefore
|
||||||
// process() call will have undefined behavior!
|
// process() call will have undefined behavior!
|
||||||
|
thread_pool.queue_work([&]{ process(local); });
|
||||||
}
|
}
|
||||||
|
|
||||||
##### Example, good
|
##### Example, good
|
||||||
|
|
||||||
{
|
{
|
||||||
int local = 42;
|
int local = 42;
|
||||||
thread_pool.queue_work([=]{ process(local); }); // Want a copy of local.
|
// Want a copy of local.
|
||||||
// Since a copy of local is made, it will be
|
// Since a copy of local is made, it will be
|
||||||
// available at all times for the call.
|
// available at all times for the call.
|
||||||
|
thread_pool.queue_work([=]{ process(local); });
|
||||||
}
|
}
|
||||||
|
|
||||||
##### Enforcement
|
##### Enforcement
|
||||||
|
@ -3212,8 +3239,9 @@ It's confusing. Writing `[=]` in a member function appears to capture by value,
|
||||||
// ...
|
// ...
|
||||||
|
|
||||||
auto lambda = [=]{ use(i, x); }; // BAD: "looks like" copy/value capture
|
auto lambda = [=]{ use(i, x); }; // BAD: "looks like" copy/value capture
|
||||||
// notes: [&] has identical semantics and copies the this pointer under the current rules
|
// [&] has identical semantics and copies the this pointer under the current rules
|
||||||
// [=,this] and [&,this] are not much better, and confusing
|
// [=,this] and [&,this] are not much better, and confusing
|
||||||
|
|
||||||
x = 42;
|
x = 42;
|
||||||
lambda(); // calls use(42);
|
lambda(); // calls use(42);
|
||||||
x = 43;
|
x = 43;
|
||||||
|
@ -3949,7 +3977,8 @@ Consider a `T*` a possible owner and therefore suspect.
|
||||||
|
|
||||||
void use(Smart_ptr<int> p1)
|
void use(Smart_ptr<int> p1)
|
||||||
{
|
{
|
||||||
auto p2 = p1; // error: p2.p leaked (if not nullptr and not owned by some other code)
|
// error: p2.p leaked (if not nullptr and not owned by some other code)
|
||||||
|
auto p2 = p1;
|
||||||
}
|
}
|
||||||
|
|
||||||
Note that if you define a destructor, you must define or delete [all default operations](#Rc-five):
|
Note that if you define a destructor, you must define or delete [all default operations](#Rc-five):
|
||||||
|
@ -4035,7 +4064,7 @@ If the `Handle` owns the object referred to by `s` it must have a destructor.
|
||||||
|
|
||||||
Independently of whether `Handle` owns its `Shape`, we must consider the default copy operations suspect:
|
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 x {*new Circle{p1, 17}}; // causes a leak if the Handle is not a Circle
|
||||||
Handle y {*new Triangle{p1, p2, p3}};
|
Handle y {*new Triangle{p1, p2, p3}};
|
||||||
x = y; // the default assignment will try *x.s = *y.s
|
x = y; // the default assignment will try *x.s = *y.s
|
||||||
|
|
||||||
|
@ -4454,7 +4483,8 @@ Being able to set a value to "the default" without operations that might fail si
|
||||||
##### Example, problematic
|
##### Example, problematic
|
||||||
|
|
||||||
template<typename T>
|
template<typename T>
|
||||||
class Vector0 { // elem points to space-elem element allocated using new
|
// elem points to space-elem element allocated using new
|
||||||
|
class Vector0 {
|
||||||
public:
|
public:
|
||||||
Vector0() :Vector0{0} {}
|
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} {}
|
||||||
|
@ -4472,9 +4502,11 @@ For example, `Vector0 v(100)` costs 100 allocations.
|
||||||
##### Example
|
##### Example
|
||||||
|
|
||||||
template<typename T>
|
template<typename T>
|
||||||
class Vector1 { // elem is nullptr or elem points to space-elem element allocated using new
|
// elem is nullptr or elem points to space-elem element allocated using new
|
||||||
|
class Vector1 {
|
||||||
public:
|
public:
|
||||||
Vector1() noexcept {} // sets the representation to {nullptr, nullptr, nullptr}; doesn't throw
|
// sets the representation to {nullptr, nullptr, nullptr}; doesn't throw
|
||||||
|
Vector1() noexcept {}
|
||||||
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:
|
private:
|
||||||
|
@ -4819,7 +4851,8 @@ It is simple and efficient. If you want to optimize for rvalues, provide an over
|
||||||
public:
|
public:
|
||||||
Foo& operator=(const Foo& x)
|
Foo& operator=(const Foo& x)
|
||||||
{
|
{
|
||||||
auto tmp = x; // GOOD: no need to check for self-assignment (other than performance)
|
// GOOD: no need to check for self-assignment (other than performance)
|
||||||
|
auto tmp = x;
|
||||||
std::swap(*this, tmp);
|
std::swap(*this, tmp);
|
||||||
return *this;
|
return *this;
|
||||||
}
|
}
|
||||||
|
@ -5188,7 +5221,9 @@ To prevent slicing, because the normal copy operations will copy only the base p
|
||||||
};
|
};
|
||||||
|
|
||||||
auto d = make_unique<D>();
|
auto d = make_unique<D>();
|
||||||
auto b = make_unique<B>(d); // oops, slices the object; gets only d.data but drops d.moredata
|
|
||||||
|
// oops, slices the object; gets only d.data but drops d.moredata
|
||||||
|
auto b = make_unique<B>(d);
|
||||||
|
|
||||||
##### Example
|
##### Example
|
||||||
|
|
||||||
|
@ -5342,12 +5377,17 @@ Worse, a direct or indirect call to an unimplemented pure virtual function from
|
||||||
|
|
||||||
derived()
|
derived()
|
||||||
{
|
{
|
||||||
f(); // BAD: attempt to call an unimplemented virtual function
|
// BAD: attempt to call an unimplemented virtual function
|
||||||
|
f();
|
||||||
|
|
||||||
g(); // BAD: will call derived::g, not dispatch further virtually
|
// BAD: will call derived::g, not dispatch further virtually
|
||||||
derived::g(); // GOOD: explicitly state intent to call only the visible version
|
g();
|
||||||
|
|
||||||
h(); // ok, no qualification needed, h is final
|
// GOOD: explicitly state intent to call only the visible version
|
||||||
|
derived::g();
|
||||||
|
|
||||||
|
// ok, no qualification needed, h is final
|
||||||
|
h();
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
|
@ -5785,7 +5825,8 @@ A class with a virtual function is usually (and in general) used via a pointer t
|
||||||
// ... no user-written destructor, defaults to public nonvirtual ...
|
// ... no user-written destructor, defaults to public nonvirtual ...
|
||||||
};
|
};
|
||||||
|
|
||||||
struct D : B { // bad: class with a resource derived from a class without a virtual destructor
|
// bad: class with a resource derived from a class without a virtual destructor
|
||||||
|
struct D : B {
|
||||||
string s {"default"};
|
string s {"default"};
|
||||||
};
|
};
|
||||||
|
|
||||||
|
@ -6387,7 +6428,8 @@ It also gives an opportunity to eliminate a separate allocation for the referenc
|
||||||
|
|
||||||
##### Example
|
##### Example
|
||||||
|
|
||||||
shared_ptr<Foo> p {new<Foo>{7}}; // OK: but repetitive; and separate allocations for the Foo and shared_ptr's use count
|
// OK: but repetitive; and separate allocations for the Foo and shared_ptr's use count
|
||||||
|
shared_ptr<Foo> p {new<Foo>{7}};
|
||||||
|
|
||||||
auto q = make_shared<Foo>(7); // Better: no repetition of Foo; one object
|
auto q = make_shared<Foo>(7); // Better: no repetition of Foo; one object
|
||||||
|
|
||||||
|
@ -7329,9 +7371,10 @@ Note that it is possible to get undefined initialization order even for `const`
|
||||||
|
|
||||||
void use()
|
void use()
|
||||||
{
|
{
|
||||||
Record* p1 = static_cast<Record*>(malloc(sizeof(Record)));
|
|
||||||
// p1 may be nullptr
|
// p1 may be nullptr
|
||||||
// *p1 is not initialized; in particular, that string isn't a string, but a string-sized bag of bits
|
// *p1 is not initialized; in particular,
|
||||||
|
// that string isn't a string, but a string-sized bag of bits
|
||||||
|
Record* p1 = static_cast<Record*>(malloc(sizeof(Record)));
|
||||||
|
|
||||||
auto p2 = new Record;
|
auto p2 = new Record;
|
||||||
|
|
||||||
|
@ -7422,7 +7465,8 @@ If you perform two explicit resource allocations in one statement, you could lea
|
||||||
|
|
||||||
This `fun` can be called like this:
|
This `fun` can be called like this:
|
||||||
|
|
||||||
fun(shared_ptr<Widget>(new Widget(a, b)), shared_ptr<Widget>(new Widget(c, d))); // BAD: potential leak
|
// BAD: potential leak
|
||||||
|
fun(shared_ptr<Widget>(new Widget(a, b)), shared_ptr<Widget>(new Widget(c, d)));
|
||||||
|
|
||||||
This is exception-unsafe because the compiler may reorder the two expressions building the function's two arguments.
|
This is exception-unsafe because the compiler may reorder the two expressions building the function's two arguments.
|
||||||
In particular, the compiler can interleave execution of the two expressions:
|
In particular, the compiler can interleave execution of the two expressions:
|
||||||
|
@ -7821,18 +7865,26 @@ The following should not pass code review:
|
||||||
|
|
||||||
void my_code()
|
void my_code()
|
||||||
{
|
{
|
||||||
f(*g_p); // BAD: passing pointer or reference obtained from a nonlocal smart pointer
|
// BAD: passing pointer or reference obtained from a nonlocal smart pointer
|
||||||
// that could be inadvertently reset somewhere inside f or it callees
|
// that could be inadvertently reset somewhere inside f or it callees
|
||||||
g_p->func(); // BAD: same reason, just passing it as a "this" pointer
|
f(*g_p);
|
||||||
|
|
||||||
|
// BAD: same reason, just passing it as a "this" pointer
|
||||||
|
g_p->func();
|
||||||
}
|
}
|
||||||
|
|
||||||
The fix is simple -- take a local copy of the pointer to "keep a ref count" for your call tree:
|
The fix is simple -- take a local copy of the pointer to "keep a ref count" for your call tree:
|
||||||
|
|
||||||
void my_code()
|
void my_code()
|
||||||
{
|
{
|
||||||
auto pin = g_p; // cheap: 1 increment covers this entire function and all the call trees below us
|
// cheap: 1 increment covers this entire function and all the call trees below us
|
||||||
f(*pin); // GOOD: passing pointer or reference obtained from a local unaliased smart pointer
|
auto pin = g_p;
|
||||||
pin->func(); // GOOD: same reason
|
|
||||||
|
// GOOD: passing pointer or reference obtained from a local unaliased smart pointer
|
||||||
|
f(*pin);
|
||||||
|
|
||||||
|
// GOOD: same reason
|
||||||
|
pin->func();
|
||||||
}
|
}
|
||||||
|
|
||||||
##### Enforcement
|
##### Enforcement
|
||||||
|
@ -8017,7 +8069,7 @@ Readability. Minimize resource retention. Avoid accidental misuse of value.
|
||||||
|
|
||||||
void use(const string& name)
|
void use(const string& name)
|
||||||
{
|
{
|
||||||
string fn = name+".txt";
|
string fn = name + ".txt";
|
||||||
ifstream is {fn};
|
ifstream is {fn};
|
||||||
Record r;
|
Record r;
|
||||||
is >> r;
|
is >> r;
|
||||||
|
@ -8030,7 +8082,7 @@ In this case, it might be a good idea to factor out the read:
|
||||||
|
|
||||||
Record load_record(const string& name)
|
Record load_record(const string& name)
|
||||||
{
|
{
|
||||||
string fn = name+".txt";
|
string fn = name + ".txt";
|
||||||
ifstream is {fn};
|
ifstream is {fn};
|
||||||
Record r;
|
Record r;
|
||||||
is >> r;
|
is >> r;
|
||||||
|
@ -8241,7 +8293,8 @@ or:
|
||||||
|
|
||||||
or:
|
or:
|
||||||
|
|
||||||
double scalbn(double base, int exponent); // better: base * pow(FLT_RADIX, exponent); FLT_RADIX is usually 2
|
// better: base * pow(FLT_RADIX, exponent); FLT_RADIX is usually 2
|
||||||
|
double scalbn(double base, int exponent);
|
||||||
|
|
||||||
##### Enforcement
|
##### Enforcement
|
||||||
|
|
||||||
|
@ -9225,21 +9278,29 @@ Complicated expressions are error-prone.
|
||||||
|
|
||||||
##### Example
|
##### Example
|
||||||
|
|
||||||
while ((c = getc()) != -1) // bad: assignment hidden in subexpression
|
// bad: assignment hidden in subexpression
|
||||||
|
while ((c = getc()) != -1)
|
||||||
|
|
||||||
while ((cin >> c1, cin >> c2), c1 == c2) // bad: two non-local variables assigned in a sub-expressions
|
// bad: two non-local variables assigned in a sub-expressions
|
||||||
|
while ((cin >> c1, cin >> c2), c1 == c2)
|
||||||
|
|
||||||
for (char c1, c2; cin >> c1 >> c2 && c1 == c2;) // better, but possibly still too complicated
|
// better, but possibly still too complicated
|
||||||
|
for (char c1, c2; cin >> c1 >> c2 && c1 == c2;)
|
||||||
|
|
||||||
int x = ++i + ++j; // OK: iff i and j are not aliased
|
// OK: iff i and j are not aliased
|
||||||
|
int x = ++i + ++j;
|
||||||
|
|
||||||
v[i] = v[j] + v[k]; // OK: iff i != j and i != k
|
// OK: iff i != j and i != k
|
||||||
|
v[i] = v[j] + v[k];
|
||||||
|
|
||||||
x = a + (b = f()) + (c = g()) * 7; // bad: multiple assignments "hidden" in subexpressions
|
// bad: multiple assignments "hidden" in subexpressions
|
||||||
|
x = a + (b = f()) + (c = g()) * 7;
|
||||||
|
|
||||||
x = a & b + c * d && e ^ f == 7; // bad: relies on commonly misunderstood precedence rules
|
// bad: relies on commonly misunderstood precedence rules
|
||||||
|
x = a & b + c * d && e ^ f == 7;
|
||||||
|
|
||||||
x = x++ + x++ + ++x; // bad: undefined behavior
|
// bad: undefined behavior
|
||||||
|
x = x++ + x++ + ++x;
|
||||||
|
|
||||||
Some of these expressions are unconditionally bad (e.g., they rely on undefined behavior). Others are simply so complicated and/or unusual that even good programmers could misunderstand them or overlook a problem when in a hurry.
|
Some of these expressions are unconditionally bad (e.g., they rely on undefined behavior). Others are simply so complicated and/or unusual that even good programmers could misunderstand them or overlook a problem when in a hurry.
|
||||||
|
|
||||||
|
@ -9596,10 +9657,15 @@ Explicit `move` is needed to explicitly move an object to another scope, notably
|
||||||
void user()
|
void user()
|
||||||
{
|
{
|
||||||
X x;
|
X x;
|
||||||
sink(x); // error: cannot bind an lvalue to a rvalue reference
|
// error: cannot bind an lvalue to a rvalue reference
|
||||||
sink(std::move(x)); // OK: sink takes the contents of x, x must now be assumed to be empty
|
sink(x);
|
||||||
|
// OK: sink takes the contents of x, x must now be assumed to be empty
|
||||||
|
sink(std::move(x));
|
||||||
|
|
||||||
// ...
|
// ...
|
||||||
use(x); // probably a mistake
|
|
||||||
|
// probably a mistake
|
||||||
|
use(x);
|
||||||
}
|
}
|
||||||
|
|
||||||
Usually, a `std::move()` is used as an argument to a `&&` parameter.
|
Usually, a `std::move()` is used as an argument to a `&&` parameter.
|
||||||
|
@ -9611,8 +9677,11 @@ And after you do that, assume the object has been moved from (see [C.64](#Rc-mov
|
||||||
string s2 = s1; // ok, takes a copy
|
string s2 = s1; // ok, takes a copy
|
||||||
assert(s1 == "supercalifragilisticexpialidocious"); // ok
|
assert(s1 == "supercalifragilisticexpialidocious"); // ok
|
||||||
|
|
||||||
string s3 = move(s1); // bad, if you want to keep using s1's value
|
// bad, if you want to keep using s1's value
|
||||||
assert(s1 == "supercalifragilisticexpialidocious"); // bad, assert will likely fail, s1 likely changed
|
string s3 = move(s1);
|
||||||
|
|
||||||
|
// bad, assert will likely fail, s1 likely changed
|
||||||
|
assert(s1 == "supercalifragilisticexpialidocious");
|
||||||
}
|
}
|
||||||
|
|
||||||
##### Example
|
##### Example
|
||||||
|
@ -9923,18 +9992,21 @@ This also applies to `%`.
|
||||||
##### Example; bad
|
##### Example; bad
|
||||||
|
|
||||||
double divide(int a, int b) {
|
double divide(int a, int b) {
|
||||||
return a/b; // BAD, should be checked (e.g., in a precondition)
|
// BAD, should be checked (e.g., in a precondition)
|
||||||
|
return a / b;
|
||||||
}
|
}
|
||||||
|
|
||||||
##### Example; good
|
##### Example; good
|
||||||
|
|
||||||
double divide(int a, int b) {
|
double divide(int a, int b) {
|
||||||
Expects(b != 0); // good, address via precondition (and replace with contracts once C++ gets them)
|
// good, address via precondition (and replace with contracts once C++ gets them)
|
||||||
|
Expects(b != 0);
|
||||||
return a / b;
|
return a / b;
|
||||||
}
|
}
|
||||||
|
|
||||||
double divide(int a, int b) {
|
double divide(int a, int b) {
|
||||||
return b ? a/b : quiet_NaN<double>(); // good, address via check
|
// good, address via check
|
||||||
|
return b ? a / b : quiet_NaN<double>();
|
||||||
}
|
}
|
||||||
|
|
||||||
**Alternative**: For critical applications that can afford some overhead, use a range-checked integer and/or floating-point type.
|
**Alternative**: For critical applications that can afford some overhead, use a range-checked integer and/or floating-point type.
|
||||||
|
@ -10459,7 +10531,8 @@ C++ implementations tend to be optimized based on the assumption that exceptions
|
||||||
|
|
||||||
##### Example, don't
|
##### Example, don't
|
||||||
|
|
||||||
int find_index(vector<string>& vec, const string& x) // 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)
|
||||||
{
|
{
|
||||||
try {
|
try {
|
||||||
for (int i =0; i < vec.size(); ++i)
|
for (int i =0; i < vec.size(); ++i)
|
||||||
|
@ -12562,7 +12635,8 @@ There are three major ways to let calling code customize a template.
|
||||||
template<class T>
|
template<class T>
|
||||||
void test(T t)
|
void test(T t)
|
||||||
{
|
{
|
||||||
f(t); // require f(/*T*/) be available in caller's scope or in T's namespace
|
// require f(/*T*/) be available in caller's scope or in T's namespace
|
||||||
|
f(t);
|
||||||
}
|
}
|
||||||
|
|
||||||
* Invoke a "trait" -- usually a type alias to compute a type, or a `constexpr` function to compute a value, or in rarer cases a traditional traits template to be specialized on the user's type.
|
* Invoke a "trait" -- usually a type alias to compute a type, or a `constexpr` function to compute a value, or in rarer cases a traditional traits template to be specialized on the user's type.
|
||||||
|
@ -12570,7 +12644,8 @@ There are three major ways to let calling code customize a template.
|
||||||
template<class T>
|
template<class T>
|
||||||
void test(T t)
|
void test(T t)
|
||||||
{
|
{
|
||||||
test_traits<T>::f(t); // require customizing test_traits<> to get non-default functions/types
|
// require customizing test_traits<> to get non-default functions/types
|
||||||
|
test_traits<T>::f(t);
|
||||||
test_traits<T>::value_type x;
|
test_traits<T>::value_type x;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@ -13065,13 +13140,15 @@ Use the least-derived class that has the functionality you need.
|
||||||
void j();
|
void j();
|
||||||
};
|
};
|
||||||
|
|
||||||
void myfunc(derived1& param) // bad, unless there is a specific reason for limiting to derived1 objects only
|
// bad, unless there is a specific reason for limiting to derived1 objects only
|
||||||
|
void myfunc(derived1& param)
|
||||||
{
|
{
|
||||||
use(param.f());
|
use(param.f());
|
||||||
use(param.g());
|
use(param.g());
|
||||||
}
|
}
|
||||||
|
|
||||||
void myfunc(base& param) // good, uses only base interface so only commit to that
|
// good, uses only base interface so only commit to that
|
||||||
|
void myfunc(base& param)
|
||||||
{
|
{
|
||||||
use(param.f());
|
use(param.f());
|
||||||
use(param.g());
|
use(param.g());
|
||||||
|
@ -13844,8 +13921,10 @@ Use of these casts can violate type safety and cause the program to access a var
|
||||||
derived1 d1;
|
derived1 d1;
|
||||||
base* p = &d1; // ok, implicit conversion to pointer to base is fine
|
base* p = &d1; // ok, implicit conversion to pointer to base is fine
|
||||||
|
|
||||||
derived2* p2 = static_cast<derived2*>(p); // BAD, tries to treat d1 as a derived2, which it is not
|
// BAD, tries to treat d1 as a derived2, which it is not
|
||||||
cout << p2->get_s(); // tries to access d1's nonexistent string member, instead sees arbitrary bytes near d1
|
derived2* p2 = static_cast<derived2*>(p);
|
||||||
|
// tries to access d1's nonexistent string member, instead sees arbitrary bytes near d1
|
||||||
|
cout << p2->get_s();
|
||||||
|
|
||||||
##### Example, bad
|
##### Example, bad
|
||||||
|
|
||||||
|
@ -13902,18 +13981,29 @@ Sometimes you may be tempted to resort to `const_cast` to avoid code duplication
|
||||||
|
|
||||||
class foo {
|
class foo {
|
||||||
bar mybar;
|
bar mybar;
|
||||||
public: // BAD, duplicates logic
|
public:
|
||||||
bar& get_bar() { /* complex logic around getting a non-const reference to mybar */ }
|
// BAD, duplicates logic
|
||||||
const bar& get_bar() const { /* same complex logic around getting a const reference to mybar */ }
|
bar& get_bar() {
|
||||||
|
/* complex logic around getting a non-const reference to mybar */
|
||||||
|
}
|
||||||
|
|
||||||
|
const bar& get_bar() const {
|
||||||
|
/* same complex logic around getting a const reference to mybar */
|
||||||
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
Instead, prefer to share implementations. Normally, you can just have the non-`const` function call the `const` function. However, when there is complex logic this can lead to the following pattern that still resorts to a `const_cast`:
|
Instead, prefer to share implementations. Normally, you can just have the non-`const` function call the `const` function. However, when there is complex logic this can lead to the following pattern that still resorts to a `const_cast`:
|
||||||
|
|
||||||
class foo {
|
class foo {
|
||||||
bar mybar;
|
bar mybar;
|
||||||
public: // not great, non-const calls const version but resorts to const_cast
|
public:
|
||||||
bar& get_bar() { return const_cast<bar&>(static_cast<const foo&>(*this).get_bar()); }
|
// not great, non-const calls const version but resorts to const_cast
|
||||||
const bar& get_bar() const { /* the complex logic around getting a const reference to mybar */ }
|
bar& get_bar() {
|
||||||
|
return const_cast<bar&>(static_cast<const foo&>(*this).get_bar());
|
||||||
|
}
|
||||||
|
const bar& get_bar() const {
|
||||||
|
/* the complex logic around getting a const reference to mybar */
|
||||||
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
Although this pattern is safe when applied correctly, because the caller must have had a non-`const` object to begin with, it's not ideal because the safety is hard to enforce automatically as a checker rule.
|
Although this pattern is safe when applied correctly, because the caller must have had a non-`const` object to begin with, it's not ideal because the safety is hard to enforce automatically as a checker rule.
|
||||||
|
@ -13963,8 +14053,10 @@ 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* p = &d1; // ok, implicit conversion to pointer to base is fine
|
||||||
|
|
||||||
derived2* p2 = (derived2*)(p); // BAD, tries to treat d1 as a derived2, which it is not
|
// BAD, tries to treat d1 as a derived2, which it is not
|
||||||
cout << p2->get_s(); // tries to access d1's nonexistent string member, instead sees arbitrary bytes near d1
|
derived2* p2 = (derived2*)(p);
|
||||||
|
// tries to access d1's nonexistent string member, instead sees arbitrary bytes near d1
|
||||||
|
cout << p2->get_s();
|
||||||
|
|
||||||
void f(const int& i) {
|
void f(const int& i) {
|
||||||
(int&)(i) = 42; // BAD
|
(int&)(i) = 42; // BAD
|
||||||
|
@ -15440,15 +15532,18 @@ To avoid extremely hard-to-find errors. Dereferencing such a pointer is undefine
|
||||||
string* bad() // really bad
|
string* bad() // really bad
|
||||||
{
|
{
|
||||||
vector<string> v = { "this", "will", "cause" "trouble" };
|
vector<string> v = { "this", "will", "cause" "trouble" };
|
||||||
return &v[0]; // leaking a pointer into a destroyed member of a destroyed object (v)
|
// leaking a pointer into a destroyed member of a destroyed object (v)
|
||||||
|
return &v[0];
|
||||||
}
|
}
|
||||||
|
|
||||||
void use()
|
void use()
|
||||||
{
|
{
|
||||||
string* p = bad();
|
string* p = bad();
|
||||||
vector<int> xx = {7, 8, 9};
|
vector<int> xx = {7, 8, 9};
|
||||||
string x = *p; // undefined behavior: x may not be "this"
|
// undefined behavior: x may not be "this"
|
||||||
*p = "Evil!"; // undefined behavior: we don't know what (if anything) is allocated a location p
|
string x = *p;
|
||||||
|
// undefined behavior: we don't know what (if anything) is allocated a location p
|
||||||
|
*p = "Evil!";
|
||||||
}
|
}
|
||||||
|
|
||||||
The `string`s of `v` are destroyed upon exit from `bad()` and so is `v` itself. The returned pointer points to unallocated memory on the free store. This memory (pointed into by `p`) may have been reallocated by the time `*p` is executed. There may be no `string` to read and a write through `p` could easily corrupt objects of unrelated types.
|
The `string`s of `v` are destroyed upon exit from `bad()` and so is `v` itself. The returned pointer points to unallocated memory on the free store. This memory (pointed into by `p`) may have been reallocated by the time `*p` is executed. There may be no `string` to read and a write through `p` could easily corrupt objects of unrelated types.
|
||||||
|
|
Loading…
Reference in New Issue
Block a user