Solution
"Lions and tigers and bears, oh my!"
?span class="docEmphasis">桪orothy
Compared with what's wrong with this Item's code, Dorothy had nothing to complain about. Let's consider it line by line.
#include <cassert>
#include <iostream>
#include <typeinfo>
#include <string>
using namespace std;
// The following lines come from other header files.
//
char* itoa( int value, char* workArea, int radix );
extern int fileIdCounter;
The presence of a global variable should already put us on the lookout for clients that might try to use it before it has been initialized. The order of initialization for global variables (including class statics) between translation units is undefined.
Guideline
 |
Avoid using global or static objects. If you must use a global or static object, always be very careful about the order-of-initialization rules.
|
// Helpers to automate class invariant checking.
//
template<class T>
inline void AAssert( T& p )
{
static int localFileId = ++fileIdCounter;
Aha! And here we have a case in point. Say the definition of fileIdCounter is something like the following:
int fileIdCounter = InitFileId(); // starts count at 100
If the compiler happens to initialize fileIdCounter before it initializes any AAssert<T>::localFileId, well and good?TT>localFileId will get the expected value. Otherwise, the value set will be based on fileIDCounter's pre-initialization value梟amely, 0 for builtin types, and localFileId will end up with a value of 100 less than expected.
if( !p.Invariant() )
{
cerr << "Invariant failed: file " << localFileId
<< ", " << typeid(p).name()
<< " at " << static_cast<void*>(&p) << endl;
assert( false );
}
}
template<class T>
class AInvariant
{
public:
AInvariant( T& p ) : p_(p) { AAssert( p_ ); }
~AInvariant() { AAssert( p_ ); }
private:
T& p_;
};
#define AINVARIANT_GUARD AInvariant<AIType> invariantChecker( *this )
These helpers are an interesting idea in which any client class that would like to automatically check its class invariants before and after function calls simply writes a typedef of AIType to itself, then writes AINVARIANT_GUARD; as the first line of member functions. Not entirely bad, in itself.
In the client code below, these ideas unfortunately go astray. The main reason is that AInvariant hides calls to assert(), which will be automatically removed by the compiler when the program is built in non-debug mode. The following client code was likely written by a programmer who wasn't aware of this build dependency and the resulting change in side effects.
//-------------------------------------------------------------
template<class T>
class Array : private ArrayBase, public Container
{
typedef Array AIType;
public:
Array( size_t startingSize = 10 )
: Container( startingSize ),
ArrayBase( Container::GetType() ),
This constructor's initializer list has two potential errors. This first one is not necessarily an error, but was left in as a bit of a red herring.
If GetType() is a static member function, or a member function that does not use its this pointer (that is, uses no member data) and does not rely on any side effects of construction (for example, static usage counts), then this is merely poor style, but it will run correctly. Otherwise (mainly, if GetType() is a normal nonstatic member function), we have a problem. Nonvirtual base classes are initialized in left-to-right order as they are declared, so ArrayBase is initialized before Container. Unfortunately, that means we're trying to use a member of the not-yet-initialized Container base subobject.
Guideline
|
Always list base classes in a constructor's initialization list in the same order in which they appear in the class definition.
|
used_(0),
size_(startingSize),
buffer_(new T[size_])
This is a serious error, because the variables will actually be initialized in the order in which they appear later in the class definition:
buffer_(new T[size_])
used_(0),
size_(startingSize),
Writing it this way makes the error obvious. The call to new[] will make buffer an unpredictable size梩ypically zero or something extremely large, depending on whether the compiler happens to initialize object memory to nulls before invoking constructors. At any rate, the initial allocation is unlikely to end up actually being for startingSize bytes.
Guideline
|
Always list the data members in a constructor's initialization list in the same order in which they appear in the class definition.
|
{
AINVARIANT_GUARD;
}
We have a minor efficiency issue: The Invariant() function will be needlessly called twice, once during construction and again during destruction of the hidden temporary. This is a nit, though, and is unlikely to be a real issue.
void Resize( size_t newSize )
{
AINVARIANT_GUARD;
T* oldBuffer = buffer_;
buffer_ = new T[newSize];
copy( oldBuffer, oldBuffer+min(size_,newSize), buffer_ );
delete[] oldBuffer;
size_ = newSize;
}
There is a control flow problem here. Before reading on, examine the function again to see if you can spot a (hint: pretty obvious) control flow problem.
The answer is: This function is not exception-safe. If the call to new[] throws a bad_alloc exception, nothing bad happens; there's no leak in this particular case. However, if a T copy assignment operator throws an exception (in the course of the copy() operation), then not only is the current object left in an invalid state, but the original buffer is leaked, because all pointers to it are lost and so it can never be deleted.
The point of this function was to show that few, if any, programmers yet write exception-safe code as a matter of habit.
Guideline
|
Always endeavor to write exception-safe code. Always structure code so that resources are correctly freed and data is in a consistent state even in the presence of exceptions.
|
string PrintSizes()
{
AINVARIANT_GUARD;
char buf[30];
return string("size = ") + itoa(size_,buf,10) +
", used = " + itoa(used_,buf,10) ;
}
The prototyped itoa() function uses the passed buffer as a scratch area. However, there's a control flow problem. There's no way to predict the order in which the expressions in the last line are evaluated, because the order in which function parameters are ordered is undefined and implementation-dependent.
What the last line really amounts to is something like this, since operator+() calls are still performed left-to-right:
return
operator+(
operator+(
operator+( string("size = "),
itoa(size_,buf,10) ) ,
", used = " ) ,
itoa(used_,buf,10) );
Say that size_ is 10 and used_ is 5. Then, if the outer operator+()'s first parameter is evaluated first, the output will be the correct "size = 10, used = 5" because the results of the first itoa() is used and stored in a temporary string before the second itoa() reuses the same buffer. If the outer operator+()'s second parameter is evaluated first (as it is on certain popular compilers), the output will be the incorrect "size = 10, used = 10" because the outer itoa() is executed first, then the inner itoa() will clobber the results of the outer itoa() before either value is used.
Common Mistake
 |
Never write code that depends on the order of evaluation of function arguments.
|
bool Invariant()
{
if( used_ > 0.9*size_ ) ; Resize( 2*size_ ) ;
return used_ <= size_;
}
The call to Resize() has two problems.
In this case, the program wouldn't work at all anyway, because if the condition is true, then Resize() will be called, only to immediately call Invariant() again, which will find the condition still true and will call Resize() again, which梬ell, you get the idea. What if, for efficiency, the writer of AAssert() decided to remove the error reporting and simply wrote "assert( p->Invariant() );"? Then this client code becomes deplorable style, because it puts code with side effects inside an assert() call. This means the program's behavior will be different when compiled in debug mode than it is when compiled in release mode. Even without the first problem, this would be bad because it means that Array objects will resize at different times ,depending on the build mode. That will make the testers' lives a torture as they try to reproduce customer problems on a debug build that ends up having a different run-time memory image characteristics.
The bottom line is: Never write code with side effects inside a call to assert() (or something that might be one), and always make sure your recursions really terminate.
private:
T* buffer_;
size_t used_, size_;
};
int f( int& x, int y = x ) { return x += y; }
The second parameter default isn't legal C++ at any rate, so this shouldn't compile under a conforming compiler (though some systems will take it). For the rest of this discussion, assume that the default value for y is 1.
int g( int& x ) { return x /= 2; }
int main( int, char*[] )
{
int i = 42;
cout << "f(" << i << ") = " << f(i) << ", "
<< "g(" << i << ") = " << g(i) << endl;
Here we run into parameter evaluation ordering again. Since there's no telling the order in which f(i) or g(i) will be executed (or, for that matter, the ordering of the two bald evaluations of i itself), the printed results can be quite incorrect. One example result is MSVC's "f(22) = 22, g(21) = 21", which means the compiler is likely evaluating all function arguments in order from right to left.
But isn't the result wrong? No, the compiler is right梐nd another compiler could print out something else and still be right, too, because the programmer is relying on something that is undefined in C++.
Common Mistake
|
Never write code that depends on the order of evaluation of function arguments.
|
Array<char> a(20);
cout << a.PrintSizes() << endl;
}
This should, of course, print "size = 20, used = 0", but because of the already-discussed bug in PrintSizes(), some popular compilers print "size = 20, used = 20", which is clearly incorrect.
Perhaps Dorothy wasn't quite right after all about her menagerie of animals梩he following might be closer:
"Parameters and globals and exceptions, oh my!"
?span class="docEmphasis">?Dorothy, after an intermediate C++ course
|
