|
|
Answer 1: The problem is that a large herd contains 10,000 sheep. That's 40,000 legs. The maximum number you can fit in a short int is 32,767. That's smaller than 40,000, so (10,000*4) causes an overflow that results in wrong data being output.
Answer 2: The problem is that the statement:
// The bit we are printing now
short int bit = (1<<16);
does not set the variable bit to 1000 0000 0000 0000(b). Instead, it sets it to 1 0000 0000 0000 0000(b). Unfortunately, it can't hold 17 bits, so the result is that it's set to zero.
Because it is zero, the bit test statement will always fail, giving use the result:
---------------
Answer 3: Global classes are initialized before main. Order is not guaranteed by the compiler. In particular, there is nothing to guarantee that first_name is initialized before it is used. So if the compiler chooses the wrong order, the program will output incorrect data or die.
Answer 4: The programmer thought he put two statements inside the if, but he forgot the curly braces.
So the statement:
if (size > MAX)
std::cout << "Size is too large\n";
size = MAX;
properly indented looks like:
if (size > MAX)
std::cout << "Size is too large\n";
size = MAX;
What the programmer should have written is:
if (size > MAX)
{
std::cout << "Size is too large\n";
size = MAX;
}
Answer 5: The problem is that the file type was not specified as binary (ios::bin). The Microsoft Windows runtime library edits character output and inserts <carriage-return (0xD)> before each <line-feed (0xA)>. This explains the extra 0D in the file just before the 0A character.
Answer 6: The problem is the line:
6 void main()
The function main is not a void function. It's an int. The function returns an exit code to the operating system. A properly written "Hello World" looks like:
1 /************************************************
2 * The "standard" hello world program. *
3 ************************************************/
4 #include <ostream>
5
6 int main()
7 {
8 std::cout << "Hello world!\n";
9 return (0);
10 }
When my wife first took programming, this was the first program she was taught (the void version). I changed the void to an int and she turned the paper in. The teaching assistant counted it wrong and changed it back.
Needless to say, I was not happy about this and wrote him a very snooty letter telling him that main was an int and quoting him chapter and verse of the C++ standard proving it. He wrote back and was extremely nice about the whole thing.
Answer 7: The problem is that sub.cpp defines str as a character array (char []). The extern statement in main.cpp defines str as a character pointer (char *).
Now character arrays and character pointers are interchangeable almost everywhere in C++. This is one of the few cases they are not. In this case, the program main thinks that str is a character pointer, so it goes to that location and reads the first four bytes expecting an address. The first four bytes are "Hell," which is not an address, and so the program crashes.
Avoidance 1: Always define externs in a header file. This header should always be included by the module where the item is defined and every module where it's used.
Answer 8: The problem is that ch can be a signed character. That means that if ch is 0xFF when converted to a signed integer for comparison purposes you get int(ch)=-1 (0xFFFFFFF). That's not 0xFF and the comparison fails.
Avoidance 2: Be careful when you use character variables to hold numbers. They may not do what you want them to.
Answer 9: The problem is that the optimizer looks at the code and sees that we read *in_port_ptr three times and then throws away the result. The optimizer then figures out that it can optimize the program and produce the same apparent results by taking out the lines 20, 21, and 22.
The solution is to declare the port pointers volatile. In Program 107 we've done this, but something is not quite right.
Answer 10: The answer is that the printf format (%d) does not match the parameter type (double). The programmer should have written:
12 printf("pi is %f\n", M_PI);
Answer 11: A character has 8 bits numbered 0 to 7. The bits can be represented by the constants (1 << 0) to (1 << 7).
There is no bit number 8, so the expression
privs |= P_BACKUP; // P_BACKUP = (1 << 8)
does nothing because it sets a bit outside the boundary of the character. The result is that only the administration privilege is really set.
Answer 12: The operator = function call takes a single parameter of type data_holder. This type of parameter is a call by value parameter, so the copy constructor is called. The programmer making the copy constructor decided to take a shortcut and uses the operator = to implement the copy. So operator = calls the copy constructor, which calls operator = which calls the copy constructor ... and so on until you run out of stack.
The operator = function should take a constant reference as its parameter type:
data_holder &operator = (
const data_holder &old_data_holder) {
It should also return a reference to a data holder.
Avoidance 3: Use const references if possible when passing parameters. This avoids the extra cost of doing a copy of a call by value parameter.
Answer 13: The problem is with the if statement. In the first one:
if (width < MIN) {
std::cout << "Width is too small\n";
width = MIN;
the programmer forgot to put in the closing curly brace. That's OK; he made up for it by forgetting to put in an opening brace for the next if statement:
if (height < MIN)
std::cout << "Height is too small\n";
height = MIN;
}
If we properly indent the code, we can see the problem:
if (width < MIN) {
std::cout << "Width is too small\n";
width = MIN;
if (height < MIN)
std::cout << "Height is too small\n";
height = MIN;
}
What the programmer should have written is:
if (width < MIN) {
std::cout << "Width is too small\n";
width = MIN;
}
if (height < MIN) {
std::cout << "Height is too small\n";
height = MIN;
}
save_queue = a_queue
copies a queue of size 30 to a queue of size 20. In other words, the assignment operator (as implemented) allows us to copy different size queue. We should not be allowed to do this.
There are four ways to solve this problem:
Use the STL queue class.
Make the assignment operator private (and not allow any assignments).
Change the assignment operator so that it throws an exception if the size of the queue is not the same.
Change the queue class so that you can assign different size queues to each other.
Answer 15: The constant 02126 is octal because the leading digit is a zero. So in C++, 02126 (octal) is 1110 (decimal) and is not the zip code for Boston.
Answer 16: The problem is that the compiler knows what 12 * 34 equals, so instead of doing the multiply it optimizes the statement and turns it into:
18 result = 408;
Since the multiply is not done, the timing is off. Program 109 is an attempt to fix this problem.
Answer 17: The problem is that the programmer used bitwise and (&) instead of logical and (&&). A bitwise and of the two numbers gives us:
3 0011
& 12 1100
=========
0 0000
So the result is 0, the if clause is skipped, and the else clause is executed.
Some programmers use the shorthand:
if (x)
for
if (x != 0)
(I discourage such shorthand.)
This is one example of why I don't like shortcuts. A better way of writing the if statement is:
if ((i1 != 0) && (i2 != 0))
Shortly after discovering this bug I told a colleague about it. I explained what happened and said, "I now know the difference between 'and' and 'and and'." I'm not sure what amazed me more, the fact that I came up with this sentence or the fact the he understood it.
Answer 18: The problem is that tmp_name returns a pointer to the local variable name. When the function ends, the storage for all nonstatic local variables is reclamined. This includes the storage for name. Thus, the pointer returned points to a random, unallocated section of memory.
The next function call that comes along will probably clobber that storage and make a_name look really strange.
A solution to this problem is to declare name static.
(See Program 59 for a similar problem.)
Answer 19: The problem is that the statement
bit >>= 1;
does not move the bit over to the right one. Instead it does a "signed" shift, which copies the sign bit. Thus
0x8000 >> 1 1000 0000 0000 0000 (b)
is not
0x4000 0100 0000 0000 0000 (b)
as expected but instead
0xC000 1100 0000 0000 0000 (b)
Because of this problem, the bit testing gives incorrect results.
Answer 20: The arguments to memset are:
memset(
void *ptr,// Pointer to the data
int value,// Value to set
size_t size// Number of bytes to fill
);
In this case, the value is sizeof(array) and the number of bytes to fill is 0. Since size=0 nothing was done.
The programmer should have written:
memset(array, '\0', sizeof(array));
Answer 21: The C++ standard states that all pointers must point to the array or above. You can't point below the array.
In this example, we have an array on an Intel machine. The address of the array, in Intel strange pointer parlance, is:
5880:0000
The data_ptr variable starts out at:
5880:001E
It then gets decremented as long as it is greater than data. During its decrementation data_ptr goes to
5880:0000
That's equal to the address of the array data, so it's decremented again. (Remember that in this memory model, only the address part is changed.) The result is:
5880:FFFE
Now
data_ptr >= data
is evaluated. But data_ptr is now much greater than data, so the program continues.
The result is that the program writes over random data, which can cause the system to crash. But if it doesn't, data_ptr will go down to:
5880:0000
wrap, and the process will continue again.
Answer 22: The problem is that the function max returns a reference to a parameter. That parameter is 3+4, which is an expression.
What C++ actually does when min is called is:
Creates a temporary (tmp1) and assigns it 1+2
Creates a temporary (tmp2) and assigns it 3+4
Calls max(tmp1, tmp2)
This function returns a reference to tmp2.
i = &tmp2 tmp1 destroyed tmp2 destroyed
The variable i is now a reference to nothing.
The problem is caused by returning a reference to a parameter. This creates a dangling reference.
Answer 23: The programmer did not put spaces in the output text for the line:
13 std::cout << "Twice" << number << "is" <<
14 (number *2) << '\n';
as a result, the output looks like
Twice5is10
What he should have written is:
Answer 24: This is a classic deadlock problem:
Process 1 requires resources #1 and #2.
Process 2 requires resources #2 and #1.
They get the resources in that order. Remember that thread switches can occur at any time.
So we have a race condition in which the following can occur:
Process 1 gets resource #1
Thread switch to process 2
Process 2 gets resource #2
Process 2 attempts to get resource #1
Resource #1 is unavailable, so the process sleeps until it is freed (keeping resource #2 locked while it works)
Thread switch to process 1
Process 1 attempts to get resource #2. It's locked, so the process sleeps until it is freed. (Resource #1 is kept locked in the meantime.)
The result is that process 1 is waiting for resource #2 while holding resource #1. It will not give up resource #1 until it gets resource #2.
Process 2 is waiting for resource #1 while holding resource #2. It will not give up resource #2 until it gets process #1.
Avoidance 4: Define locking order (for example, you must get the locks in the order #1, #2). Always use this locking order when getting multiple locks.
Alternate: When getting multiple locks, use the following algorithm:
Attempt to get all the locks (do not block if they are not available).
If you've got everything, then go on and do your job.
If you didn't get all the locks, free the ones you didn't get, sleep a while, and try again.
Answer 25: The problem is the statement:
if (n2 =! 0)
This is an assignment statement inside an if. If we rewrite the code to avoid the shortcut, we get the two statements.:
n2 = !0;
if (n2)
The use of the logical not in this context ( !0 ) gives us a result of 1. So we always assign n2 the value 1, then do the comparison and divide.
The != was written backwards as =! thus giving us the surprise.
The statement should have read:
if (n2 != 0)
diff[diff_index++] =
array[i++] - array[i++];
This tells the compiler to:
Increment i
Use it to index array (first occurrence)
Increment i
Use it to index array (second occurrence)
Compute the difference
The problem is that steps 1-4 can occur in a different order:
Increment i
Increment i
Use it to index array (first occurrence)
Use it to index array (second occurrence)
Statements with many side effects give the C++ compiler latitude to screw things up.
Avoidance 5: Put side effects like ++ and -- on lines by themselves.
Answer 27: The problem is that "1" is an integer. The number "3" is also an integer. So "1/3" is an integer divide.
Thus, the statement:
12 result = 1/3; // Assign result something
does an integer divide of 1 by 3. Integer divides truncate the fractional part so the result is 0. The integer "0" is turned into floating-point and assigned result.
The programmer should have written this as:
12 result = 1.0 / 3.0;// Assign result something
Answer 28: The scanf function is extremely tricky to use. In this program the statement:
22 scanf("%c %d", &oper, &value);
gets a character and a integer. The next time scanf is called, it will read another character and integer. So what's the next character? Let's look at the sample run:
% calc
Enter operator and value:+ 5
Total: 5
Enter operator and value:+ 10
Bad operator entered
Total: 5
Enter operator and value:Bad operator entered
Total: 5
Enter operator and value:q
Bad operator entered
Total: 5
Enter operator and value:q
The first line we type is:
+ 5
After the first scanf call, the input pointer is position just before the newline just after the 5. The next scanf tries to read the operator and gets the newline. It keeps reading and sees a + instead of a number. The result is a lot of confusion.
Avoidance 6: The scanf function is tricky to get right. But I have a simple way of dealing with this problem: I never use it. Instead I always use a combination of fgets and sscanf instead.
fgets(line, sizeof(line), stdin);
sscanf(line, "%c %d", &operator, &value);
Answer 29: The preprocessor does not understand C++ syntax. When we define TOTAL to be 37 + 33, it is literally 37 + 33 and not 70.
The AREA macro is defined as:
37 + 33 * 10
Operator precedence takes over and gives us the wrong answer.
Avoidance 7: Use constants instead of defined macros whenever possible.
Avoidance 8: Put parenthesis around all #defines that define anything other than a simple number.
Example:
// Total top size
#define TOP_TOTAL (TOP_PART1 + TOP_PART2)
Answer 30: The problem is that the function is returning a reference to a local variable. This is a bad thing because the local variable is destroyed by the return; the reference is what is called a dangling reference. It's referring to something that is no longer there.
When we try to print the string that is no longer there, we run into trouble.
Avoidance 9: Do not return references to local variables.
Answer 31: The problem is that the else clause goes with the nearest if. The properly indented code looks like:
23 if (balance < 0)
24 if (balance < - (100*DOLLAR))
25 cout << "Credit " << -balance << endl;
26 else
27 cout << "Debt " << balance << endl;
This is not what the programmer intented. What he wanted to do was:
if (balance < 0) {
if (balance < - (100*DOLLAR))
cout << "Credit " << -balance << endl;
} else
cout << "Debt " << balance << endl;
Avoidance 10: Use {} around statements under the control of an if, for, while, or other control statement if there is more than one statement conditional control.
(That's a fancy way of saying: Don't write code like this.)
 |
Bonus question: This fixes most of the problems, but there's still a bug in this program. What is it? (Next Hint 112. Answer 38.)
|
Answer 32: The problem is that memory is allocated in the constructor and never freed.
Avoidance 11: Always delete in the destructor what you new in the constructor.
This rule was not followed, so every time we created a stack some of the heap permanently went away.
Answer 33: The program prints:
First: John
Last: Smith
Hello: John
Smith
The problem is that fgets gets a line including the newline. So when the first name is read, it's read as John\n. The same thing happens with Smith, and the result is our funny output.
Answer 34: There is a extra semicolon at the end of the for statement:
for (index = 1; index <= 10; ++index);
This means that the for controls absolutely nothing. Properly indented the program is:
for (index = 1; index <= 10; ++index);
std::cout << index << " squared " <<
(index * index) << '\n';
or if we add a little commenting this looks like:
for (index = 1; index <= 10; ++index)
/* Do nothing */;
std::cout << index << " squared " <<
(index * index) << '\n';
From this we can see that the std::cout line is not inside the for loop.
Answer 35: The problem is that we declared a local variable named remove. There is a standard function named remove as well. Our local variable hid the function for the scope of the local variable.
That scope ended at the end of the first if on line 15.
The next statement:
16 if (remove) {
checks to see if the address of the function remove is non-zero and executes the next statement if it is.
Avoidance 12: Avoid hidden variables.
Answer 36: The problem is that the string we return is defined as:
15 // The name we are generating
16 std::string name;
This is a local variable. The subroutine returns a reference to this string. But because it's a local variable, it's destroyed at the end of the function. That means when we use the result, the variable holding the result has been destroyed.
Answer 37: The problem is that the backslash character is used as an escape character. So \n is newline. \new is <newline>ew.
So the string \root\new\table decodes as
"<return>oot<newline>ew<tab>able"
What the programmer really wanted was:
const char name[] = "\\root\\new\\table"; // DOS path
Ironically, this rule does not apply to #include file names so
#include "\usr\include\table.h"
works and is correct.
Answer 38: The problem is the statement:
if (balance < 0)
This is used to check to see if the customer owes the company something. Thus, the customer can see a message like:
You owe 0.
| Note |
This actually happened to one person. He got a bill for $0.00. He called up the company, they apologized, and the next month he got a bill for $0.00. This continued for many months. Each time he called the company, they would apologize and tell him they would fix the problem, but nothing would happen. |
He even got charged a late fee of 5%. This brought his bill up to $0.00.
Finally, he sent them a check for $0.00.
That week he got a nasty phone call from his bank. "Why did you write out such a check?" they demanded to know.
It seems that the check crashed its computer system. So the check was bounced, and the next week he received a bill for $0.00.
Answer 39: The problem is that the optimizer is smart. It sees that we are computing the result of
factor1 * factor2;
inside the for loop. The answer won't change if we move this to outside the for loop, but things will go quicker. So the optimized version of this program does the multiply only one time:
17 int register1 = factor1 * factor2;
18 // We know that 1863 multiplies
19 // delay the proper amount
20 for (i = 0; i < 1863; ++i)
21 {
22 result = register1;
23 }
To fix this problem we need to declare our factor volatile.
1 /************************************************
2 * bit_delay -- Delay one bit time for *
3 * serial output. *
4 * *
5 * Note: This function is highly system *
6 * dependent. If you change the *
7 * processor or clock it will go bad. *
8 *************************************************/
9 void bit_delay(void)
10 {
11 int i; // Loop counter
12 volatile int result;// Result of the multiply
13
14 // Factors for multiplication
15 volatile int factor1 = 12;
16 volatile int factor2 = 34;
17
18 // We know that 1863 multiples delay
19 // the proper amount
20 for (i = 0; i < 1863; ++i)
21 {
22 result = factor1 * factor2;
23 }
24 }
It's things like this that make embedded programming so simple.
Answer 40: The problem is that ostream is passed as "pass by value". You can't copy stream variables. (If you did it would mean that the system would have to make a copy of the file.) The parameter should be changed to a "pass by reference" parameter:
void print_msg_one(
// File to write the message to
class ostream &out_file,
// Where to send it
const char msg[]
)
Answer 41: The problem is the statement:
strcat(file_name, '/');
The strcat function takes two strings as arguments. In this example, we've given it a string and a character. Because there are no prototypes, C can't do parameter checking; the incorrect parameter is passed to strcat, which gets very confused.
Avoidance 13: All functions should be explicitly declared. Never let C declare them implicitly. Make sure you include the headers that define the prototypes for all the functions that you use.
Answer 42: A signed one-bit number can have one of two values: 0 and -1.
The statement:
printer_status.online = 1;
fails because the one-bit-wide field can't hold the value 1. (So it overflows and assigns the variable the value -1!) The result is that the next statement:
if (printer_status == 1)
fails.
Avoidance 14: Single bit fields should be unsigned.
Answer 43: On MS-DOS you'll get something like:
The answer is 4C:>#
(# is the cursor)
On UNIX you might get something like:
The answer is 4$ #
The problem is that the programmer did not add an end of line at the end of the std::cout statement. The result is that the program runs, outputs a statement, and exists leaving the cursor positioned at the end of a line. The command processor then runs and outputs its prompt (C:> for MS-DOS, $ for UNIX) right next to the program's output.
What the programmer should have written is:
std::cout << "The answer is " << result << '\n';
Answer 44: Commas can be used to separate C++ statements. It's used like:
if (x)
std::cout << "X set. Clearing\n", x = 0;
(Don't program like this, please!)
The statment
one_million = 1,000,000;
is the same as:
one_million = 1,
000,
000;
or
one_million = 1;
000;
000;
From this, we can see why we get 1 as out output.
Answer 45: The problem is that the expression ch+1 is an integer (value 66). C++ detects this and calls the std::cout.operator <<(int) function and outputs an integer.
What the programer should have written is:
std::cout << static_cast<char>(ch+1);
std::cout << static_cast<char>(ch+2);
The double of 1 is 2
The double of 2 is 3
The double of 3 is 4
The double of 4 is 5
The double of 5 is 6
The reason is that DOUBLE(i+1) expands to:
(i+1 * 2)
When C++ sees this, it multiplies 1 by 2 and adds i. This result is not what the programmer intended.
Avoidance 15: Use inline functions instead of macros whenever possible.
Avoidance 16: Always put () around the parameters of macros. Example:
#define DOUBLE(x) ((x) * 2)
if (amount = 0)
assigns 0 to amount, then compares the result to see if it's not zero. It is zero, so the else clause is executed.
The programmer should have written the statement as:
if (amount == 0)
| Note |
One of the most rewarding experiences I had when I was teaching programming was when I met a student about two months after the class had finished. "Steve," he said. "I have to tell you that during the class I thought you were going a bit overboard about this '=' vs. '==' stuff — until yesterday. You see, I wrote my first real program and guess what mistake I made?" |
i = 3 - i;
| Note |
This algorithm was first found lurking in an article as an example of how not to do the job. The author's "ideal" way of doing things was to use the following code:
switch (i) {
case 1
i = 2;
break;
case 2:
i = 1;
break;
default:
std::cerr << "Error: i is not 1 or 2\n";
exit (99)
}
The point the author was trying to make was that you should check for illegal values in your code. Sharp-eyed readers may notice that there's a syntax error in this code. There was a similar problem in the "ideal" solution in the original article. In other words, the code the author presented as "ideal" wouldn't work. |
Answer 49: The problem is that C++'s operator precedence is not what the programmer thought it was. The + operator comes before << so
y = x<<2 + 1;
gets parsed as:
y = x << (2+1);
The result is 1<<4 or 8.
Avoidance 17: Use the simple C++ precedence rules:
*, / and % come before + and -.
Put () around everything else.
Hello
Hello
The problem is that when the fork occurs, there is data in the printf buffer. The fork creates two copies of the process and two copies of the data in the printf buffer. Thus, when the buffer is flushed later (in both processes) we get a Hello from each of them.
Answer 51: The programmer never bothered to initialize sum. You can't count on a uninitialized value containing anything. So sum may start out at 0, 5190, 123, 5, or something else.
What the programmer should have written is:
9 int sum = 0;
Answer 52: The problem is the line
flags |= CD_SIGNAL;
This operation is not protected against thread switches. On a complex instruction machine, the assembly code for this looks like:
; 80x86 assembly
orb $2,flags
Thread switches occur only on an instruction boundary. So this operation cannot be interrupted on the 80x86 machine family.
But on a RISC machine such as a Sparc, the code looks a little different:
1. sethi %hi(flags),%o0 ; Get the address of the flags in %o0,%o1 2. sethi %hi(flags),%o1 3. ld [%o1+%lo(flags)],%o2 ;%o2 = contents of the variable flags 4. or %o2,2,%o1 ;%o1 = The results of seeting the flag 5. st %o1,[%o0+%lo(flags)] ;Store results in %o0
So now the C++ statement is interruptible. In particular, the following can happen:
The program runs and completes instruction 3. At this point, the value of flags is in register %o2.
A thread switch occurs.
The other process modifies flags.
The thread switches back.
The old value of flags is in register %o2.
The bit is set, and the result is stored. Because this contained the old value of flags, any changes made in the other thread are discarded accidently.
The solution to this problem is to use locks to prevent a task switch from occurring during the statement.
48 printf("%o\t", matrix[row][col]);
prints the answer in octal. The programmer made an error and put %o where he wanted %d. The result is that the numbers are correct, just in the wrong base.
Answer 54: The problem is that you can't represent 1/3 exactly in floatingpoint. Let's see what happens when we add the numbers in decimal.
1/3 = 0.33333
1/3 = 0.33333
1/3 = 0.33333
-------------
0.99999
Because of the roundoff error, the result is not 1.
Remember that when using floating-point, the numbers are not exact.
Answer 55: The problem is that we throw an exception in a destructor.
When the program reaches the line:
if (i3 < 0)
throw (problem("Bad data"));
the exception code takes charge. It destroys all the local variables. That includes the variable a_stack.
When a_stack is destroyed, the destructor is called:
~stack(void) {
if (count != 0) {
throw (problem("Stack not empty"));
}
}
The destructor throws an exception. C++ does not like it when you throw an exception in an exception. When that happens the program calls the terminate() function.
If you want to catch the second exception and other similar exception problems, use the standard function set_terminate to establish a function to take care of unexpected problems.
Avoidance 18: Don't throw exceptions in destructors.
Answer 56: The problem is that the redefined new function is implemented incorrectly. The programmer assumed that when a person does a
new fast_bit_array
the size of the allocated object is sizeof(fast_bit_array). This is not true when fast_bit_array is used as a base class. In this case, the size of the allocated memory is the size of the derived class safe_bit_array, which is bigger than fast_bit_array, thus resulting in memory confusion.
Avoidance 19: Don't define your own operator new function unless you're sure what you're doing. If you are sure you know what you're doing, make sure you're really really sure. Even then don't do it unless it's absolutely necessary.
Answer 57: The problem is that there are two variable declarations:
File: main.cpp
int value = 20;
File: check.cpp
int value = 30;
That means that the value is set to 20 or 30. But which one? The result is compiler-dependent. If you want value to be local to the files in which they are declared, you need to declare them static:
File: main.cpp
static int value = 20;
File: check.cpp
static int value = 30;
Or better yet, give them two different names.
Answer 58: According to the C++ standard, once you define a derived class member function with the same name as a base class's member function, all member functions of that name are hidden:
So der::print_it(float) hides both base: :print_it(float) and base: :print_it(int).
When we call print_it(2) C++ looks for aversion of print_it it can use. The only visible print_it is der::print_it(float). C++ would rather have a function that takes int as its argument, but it knows how to turn an int into a float, so it promotes 2 to 2.0 and uses der::print_it(float).
Answer 59: The problem is that we didn't define a copy constructor. When that happens, C++ defines one for you and generally does a bad job of it.
The copy constructor is defined as:
var_array(const var_array &other) {
data = other.data;
size = other.size;
}
The copy constructor is called to create a copy of an_array for the function store_it. The pointer to the data is copied.
When var_array::~var_array is called at the end of pushy, it returns the data to the heap.
When var_array::~var_array is called at the end of main, it returns the same data to heap. Because we delete the same memory twice, the result is a corrupt heap.
Avoidance 20: Always declare a copy constructor in some way or other. The three major was are:
Implicitly declare it.
If you never want anyone to be able to call it, declare it private:
private:
var_array (const var_array &);
// No one can copy var_arrays
If the default works, use the comment:
// Copy Constructor defaults
in your program. That way you tell people reading your code that you thought about it and know that the C++ default will not be a problem.
Answer 60: The programmer has a very bad habit of not closing files after opening them. Pretty soon the maximum number of files are opened and the system won't let him open any more.
Closes needed to be added at key points in the code:
int fd = open(cur_ent->d_name, O_RDONLY);
if (fd < 0)
continue; // Can't get the file so try again
int magic; // The file's magic number
int read_size = read(fd, &magic, sizeof(magic));
if (read_size != sizeof(magic)) {
close(fd); // <---- added
continue;
}
if (magic == MAGIC) {
close(fd); // <---- added
return (cur_ent->d_name);
}
close(fd); // <---- added
Also the programmer uses opendir to open a directory. He never closes it. So a closedir is needed.
void scan_dir(
const char dir_name[] // Directory name to use
)
{
DIR *dir_info = opendir(dir_name);
if (dir_info == NULL)
return;
chdir(dir_name);
while (1) {
char *name = next_file(dir_info);
if (name == NULL)
break;
std::cout << "Found: " << name << '\n';
}
closedir(dir_info); // <---- added
}
Answer 61: The problem is that the statement:
5 const char *volatile in_port_ptr =
6 (char *)0xFFFFFFE0;
tells C++ that the pointer is volatile. The data being pointed to is not volatile. The result is that the optimizer still optimizes us out of existence. The solution is to place the volatile where it modifies the data being pointed to. We also have added a const to the declaration to make sure that the pointer can't be modified. The resulting declarations are:
4 // Input register
5 volatile char *const in_port_ptr =
6 (char *)0xFFFFFFE0;
7
8 // Output register
10 volatile char *const out_port_ptr =
11 (char *)0xFFFFFFE1;
This tells C++:
in_port_ptr is a const pointer and cannot be modified.
*in_port_ptr is a volatile char whose value can be changed outside the normal C++ programming rules.
Answer 62: The problem is that the comment:
10 base = 5; /* Set the base of the triangle
does not contain a close comment. So it continues engulfing the statement below it:
10 base = 5; /* Set the base of the triangle
11 height = 2; /* Initialize the height */
From this it's easy to see why height was not set.
Answer 63: The problem is that getchar returns an int. We are assigning it to a character. Some systems treat characters as unsigned characters. The result is that when we get EOF (-1) the system assigns
ch = (unsigned char)(-1)
or ch = 0xFF. It then compares the 0xFF to -1 (they are not the same) and does not exit the loop.
This program is also a stylistic disaster. The goal of every C++ programmer should be writing a clear program. This program was written to be compact. A much better program is:
1 /************************************************
2 * copy -- Copy stdin to stdout. *
3 ************************************************/
4 #include <stdio.h>
5
6 int main()
7 {
8
9 while (1) {
10 {
11 int ch; // Character to copy
12
13 ch = getchar();
14
15 if (ch == EOF)
16 break;
17
18 putchar(ch);
19 }
20 return (0);
21 }
Name (a): /var/tmp/tmp.2
Name (b): /var/tmp/tmp.2
The reason for this is that although we have two pointers, they both point to one variable name. When tmp_name is called the first time:
a_name --> name = "/var/tmp/tmp.1"
After the second call:
b_name --> name = "/var/tmp/tmp.2"
But a_name also points to name so:
a_name --> name = "/var/tmp/tmp.2"
b_name --> name = "/var/tmp/tmp.2"
The second call overwrote storage that was being used to hold the result of the first call.
One solution to this is to copy the string after each call or to have the caller provide his own character array for name storage.
Another solution is to use C++ style strings that handle their own memory allocation.
Answer 65: Every put is followed by a flush. This means that a system call is made for each character output. System calls are expensive and take up a lot of CPU time.
In other words, although the I/O library is designed for buffered I/O, the excessive flush calls for it to do unbuffered I/O one character at a time.
We need to flush at the end of each block to make sure that the remote system receives a full block. That's block, not character, so we can speed up the system by moving the flush down to after the block is sent:
for (i = 0; i < BLOCK_SIZE; ++i) {
int ch;
ch = in_file.get();
serial_out.put(ch);
}
serial_out.fflush();
Answer 66: The setjmp marks a location in the code. The longjmp call jumps to it. It jumps directly to it, it does not pass go, it does not collect $200. It also skips all the destructors for all the variables on the stack. In this case, because the destructor for std::string returns the memory allocated for the string, we have a memory leak.
That's because the setjmp and longjmp functions are C functions that should not be used in C++.
Avoidance 21: Do not use setjmp and longjmp in a C++ program. Use exceptions instead.
Answer 67: In the default case:
defualt:
std::cout << i << " is not prime\n";
break;
The "default" keyword is misspelled. The result is that the C++ compiler thinks that "defualt" is a goto label.
Answer 68: The printf function buffers its output. It won't actually write anything until the buffer gets full or a newline is sent.
So the program hits the printf, the "Starting" message goes into the buffer and not to the screen, and the function average is executed and gets a divide by zero error.
The result is that the "Starting" message is lost, making us think that average was never called.
The solution to this problem is to flush the buffer explicitly after the starting message:
printf("Starting....");
fflush(stdout);
| Warning |
The rules for when a buffer gets flushed change depending on the type of file being written. The rules are:
|
(These are the rules you'll probably find on your system. The actual rules are system-dependent.)
Answer 69: The problem is the programmer wrote:
std::cout << "Hello World!/n";
instead of:
std::cout << "Hello World!\n";
so the output is literally:
Hello World/n
Answer 70: The problem is the statement:
54 while (
55 (std::strcmp(cur_cmd->cmd, cmd) != 0) &&
56 cur_cmd != NULL)
The statement checks the data pointed to by cur_cmd->cmd, then checks to see if cur_cmd->cmd is valid. On some systems, dereferencing NULL (which we do if we are at the end of the list) causes core dumps.
On MS-DOS and other brain-damaged systems, there is no memory protection, so dererferencing NULL is allowed, although you get strange results. Microsoft Windows fixed this, and dereferencing a NULL pointer will result in a General Protection Fault (GPF).
The loop should be written:
while (
(cur_cmd != NULL) &&
(std::strcmp(cur_cmd->cmd, cmd) != 0))
But even this is tricky. The statement depends on the C++ standard being correctly implemented. That C++ standard states that for && the first part is evaluated. If the first term is false, the second term is skipped. Just to be safe, it's better to write this as:
while (1) {
if (cur_cmd == NULL)
break;
if (std::strcmp(cur_cmd->cmd, cmd) == 0)
break;
Alignment
Some machines require that long integer values line up on a 2-byte or 4-byte boundary. Some do not. C++ will insert padding in the structure to make things line up.
So on one machine, the structure will be:
struct data {
char flag; // 1 byte
long int value; // 4 bytes
};
for a total of 5 bytes. While on another it may be:
struct data {
char flag; // 1 byte
char pad[3]; // 3 bytes (automatic padding)
long int value; // 4 bytes
};
for a total of 8 bytes.
Byte order
Some machines write out long integers using the byte order ABCD. Others use DCBA. This prevents things from being portable.
Integer size
The 64-bit machines are coming. That means that on some systems a long int is 64 bits, not 32.
Answer 72: We have an array of a derived class called safe stack. In C++, you can use a base class pointer (stack*) to point to a derived class (safe_stack). The system will see only the base part of the object, but you can still point to it.
Now a pointer can point to a single instance of a class or an array of objects.
So we have the following two rules:
A base pointer can point to a derived object.
An object pointer can point to an array of objects.
From this, we can conclude:
A base pointer can point to an array of derived objects.
That's wrong.
The problem is that an array of derived objects is not the same as an array of base objects.
So if we take a base pointer and point it a derived array, the memory layout will be wrong.
Avoidance 22: Use the STL vector template instead of an array. It avoids a lot of problems.
Avoidance 23: Do not pass base-class arrays as parameters.
Answer 73: The problem is how the compiler generates machine code for program.
The statement:
if (number1 + number2 == number1)
generates something like:
movefp_0, number1
add fp_0, number2
movefp_1, number1
fcmpfp_0, fp_1
jump_zero out_of_the_while
In this example fp_0 and fp_1 are floating-point registers. In floating-point coprocessors, the registers have the largest precision available. So in this case, while the numbers may be only 32-bit, the floating-point processor does things in 80 bits, resulting in a high precision being reported.
This sort of problem occurs on most machines with a floating-point processor. On the other hand, if you have an old machine that uses software to do the floating-point, you'll probably get the right answer. That's because, in general, software floating-point uses only enough bits to do the work.
To fix the program, we need to turn the main loop into:
while (1)
{
// Volatile keeps the optimizer from
// putting the result in a register
volatile float result;
result = number1 + number2;
if (result == number1)
break;
Answer 74: The problem is that the words are stored in the input file in alphabetical order and the tree is unbalanced. Thus, when words are inserted the following data structure is built up:
The result is that we have a linked list, not a tree. Words are added to the end of the linked list (expensive), and lookups are done by linear search (also expensive).
A balanced binary tree would solve this problem.
Answer 75: The problem is that we have in our code the statement:
an_array = an_array;
This is disguised as:
82 to_array = from_array;
The operator = function deletes the data of the destination array. That's fine except that the source array is the same stack, so its data gets destroyed, too.
The answer is to check explicitly for self-assignment in the operator = function:
array & operator = (const arrary &old_array) {
if (this == &old_array)
return;
Avoidance 24: The operator = function should check for self-assignment.
Answer 76: The problem is that strcmp returns 0 if the strings are equal and non-zero otherwise. That means that if you have the statement:
if (strcmp(x,y))
The if will execute only if the strings are not equal.
Avoidance 25: Use
if (strmp(x,y) != 0)
to test if two strings are equal. It's clearer than if (strcmp(x,y), and it works.
Avoidance 26: Whenever possible, use the C++ string class instead of the old C style strings. That way you can use the relational operators (<,>, ==, etc.) instead of strcmp.
Answer 77: The problem is the code:
while (first != NULL) {
delete first;
first = first->next;
}
It deletes data, then uses it. After things are deleted, they really should go away.
Avoidance 27: Always set a pointer to NULL after delete or free.
When the code is written with a little bit of added protection, the problem is obvious:
delete first
first = NULL;
first = first->next;
Also, because of the added protection of setting first to NULL, if we do attempt to use the pointer, we will abort in a well-defined manner (on most systems).
Answer 78: The types of the variables are:
sam is a character pointer (char *).
joe is a character (char).
The declaration, after the preprocessor gets through with it results in:
char * sam, joe;
Avoidance 28: Use typedef to define new types, not #define.
Answer 79: C++ has no ** operator. (At least for integers.) So (12 ** 2) is an invalid construct.
The trouble is that this bad syntax is hidden in a preprocessor macro that's not expanded until line 16. That's why line 16 is the one with the syntax error.
Avoidance 29: Use const instead of preprocessor macros whenever possible. The statement:
const int GROSS = (12 ** 2);
would still generate an error message, but at least the line number would be right.
Answer 80: The problem is that the result of a comparison is an integer 1 or 0. So the expression:
if (a > b > c)
becomes
if ((a > b) > c)
Because a is greater than b, the result of a > b is 1, so we now have
if (1 > c)
which is false, so the else clause is executed.
Answer 81: The programmer suspects that something funny is happening when data item #500 is read. He wants to put a breakpoint right before this item is read.
The trouble is that if he puts a breakpoint at the top of get_data, he will have to do 500 debugger continue commands before he reaches the point he wants.
So he puts his breakpoint at the line:
seq = seq;
| Note |
The fancier debuggers allow the user to set a skip count to skip the first x number of breakpoint stops. Our friendly programmer doesn't have such a nice tool. |
Answer 82: The programmer used semicolons to end the #define declaration. Because the preprocessor is rather literal about things, the semicolon becomes part of the text. The result is that USABLE is defined as:
8.5; -1.0;;
The initialization of text_width now becomes
double text_width = 8.5; -1.0;;
or, properly indented,
double text_width = 8.5;
-1.0;
;
From this we can see our problem.
Avoidance 30: Use const instead of #define whenever possible.
Answer 83: The problem is the buffer is a local variable. That means that it goes away at the end of the function call. Unfortunately, printf doesn't know this, so it will still stuff data into it afterwards.
The
printf("That's all\n");
will still try to use the local variable.
To fix this problem declare the buffer as static:
static char buffer[BUFSIZ];
Answer 84: The problem is the optimizer. The optimizer knows that the variable debugging is zero. It's always zero.
Now that we know that, let's take a look at the statement:
if (debugging)
This is always false, because debugging is always zero. So this block is never executed. That means that we can optimize the code:
13 if (debugging)
14 {
15 dump_variables();
16 }
into the statement:
// Nothing
Now let's look at the number of times debugging is used. It's initialized on line 11 and used on line 13. Line 13 is optimized out, so debugging is never used. If a variable is never used, it can be optimized out.
The result is an optimized program that looks like:
9 void do_work()
10 {
11 // Declaration optimized out
12
13 // Block optimized out
14 //
15 //
16 // End of block that was removed
17 // Do real work
18 }
Now our programmer wanted to use the debugging variable to help him debug things. The trouble is there is no debugging variable after optimization.
The problem is that C++ didn't know that the programmer was going to use magic (a debugger) to change variables behind its back. If you plan on doing something like this, you must tell the compiler. This is done by declaring the debugging variable volatile.
static volatile int debugging = 0;
The "volatile" keyword tells C++, "Something strange such as an interrupt routine, a debugger command, or something else may change this variable behind your back. You can make no assumptions about its value."
Answer 85: The printf statement:
11 printf("The answer is %d\n");
tells C to print an integer, but fails to supply one. The printf function doesn't know this, so it will take the next number off the stack (some random number) and print it.
What the programmer should have written is:
printf("The answer is %d\n", answer);
Answer 86: The problem is the use of matrix[1,2]. The comma operator in C++ merely returns the result of the second part. So the expression "1,2" tells C++ throw the first part (1) away and the value is 2. So matrix[1,2] is really matrix[2]. This is a pointer into an integer array, and C++ will treat it as a pointer for printing. That's why strange values get printed.
What the programmer really wanted is:
matrix[1][2]
Answer 87: The prefix version of ++ returns the number after incrementing.
Thus
++++i;
tells C++ increment i, returns the result, then increments the variable i again.
The postfix version of ++ (i++) returns a copy of the variable, then increments it.
So
++++i
Tells C++ to make a copy of i (call it tmp_1)
Increments i
Does the rest of the work on tmp_1
Makes a copy of tmp_1 (call it tmp_2)
Increments tmp_2
Returns tmp_1 as the value of the expression
| Note |
C++ won't let you get away with ++++ on integers. Only with some added class silliness can you get away with it. |
Avoidance 31: Use ++ and -- singly.
Answer 88: The problem is the macro:
#define SQR(x) ((x) * (x))
when called with
SQR(++number)
This expands to
((++number) * (++number))
This increments number twice, instead of once as the programmer intended. What's worse, the compiler can make some decisions as to the order in which the various operations are done; therefore, the result of this expression is compiler-dependent.
Avoidance 32: Use inline functions instead of parameterized macros.
Avoidance 33: Put ++ and - on lines by themselves.
Answer 89: The optimizer knows that although the subroutine computes the value of result, it does nothing with it. So the program will work the same whether or not result is computed. Thus, the optimizer takes a look at the loop:
20 for (i = 0; i < 1863; ++i)
21 {
22 result = factor1 * factor2;
23 }
is optimized down:
20 for (i = 0; i < 1863; ++i)
21 {
22 /* Do nothing */;
23 }
Of course we don't need to do nothing 1,863 times, so this is optimized down to:
20 /* No loop needed */
21 {
22 /* Do nothing */;
23 }
This is about as optimized as you can get. The way to keep the optimizer from doing this to us is to declare the variable result is volatile. Program 110 shows what happens when you add this fix.
Answer 90: C++ uses zero-based indexing. So for array [5] the valid elements are:
array[0], array[1], array[2], array[3], array[4]
The programmer, however, uses the elements 1-5. There is no array [5], so the program modifies random memory, causing the memory corruption.
That's why most C++ programs don't use statements like:
for (i = 1; i <= 5; ++i) {
Instead they count using:
for (i = 0; i < 5; ++i) {
Answer 91: The problem is that with the statement:
result=result/*divisor; /* Do divide */;
the first /* (the one in the middle of the statement) starts a comment; it does not do a divide. So this statement is:
result = result /* a very big comment */;
Avoidance 34: Put spaces around operators. It not only avoids problems but also makes the program easier to read.
result=result / *divisor; /* Do divide */;
Answer 92: The problem is that a thread switch can occur at any time.
The writer will remove a character from the buffer when count > 0. The reader performs the two steps:
++count; // We've got a new character
*in_ptr = ch;// Store the character
But a thread switch can occur between these two steps.
Therefore, the following can happen:
reader:++count;// We've got a new character
thread switch to writer
writer: check count > 0 -- it is
writer: Get the character
thread switch to reader
reader: Put the character in the buffer AFTER writer has already read it.
A solution is to change the sequence of the steps
++count; // We've got a new character
*in_ptr = ch;// Store the character
to
*in_ptr = ch;// Store the character
++count; // We've got a new character
Depending on the sequence of instructions to protect shared data is difficult and tricky.
It is much better and simpler is to tell the task manager when you are doing a set of statements that can't be interrupted. In pthreads, this is done with a mutex lock:
pthread_mutex_lock(&buffer_mutex);
++count;
*in_ptr = ch;
++in_ptr;
pthread_mutex_unlock(&buffer_mutex);
Answer 93: Member variables are initialized in declaration order.
In this case, the statements:
) : width(i_width),
height(i_height),
area(width*height)
are executed in declaration order: 1) area, 2) width, 3) height. This means that area is initialized with undefined values of width and height, and then width and height are initialized.
Avoidance 35: Write constructors so that variables are initialized in the order in which they are declared. (If you don't do this, the compiler will do it for you and cause confusion.)
Avoidance 36: Never use member variables to initialize other member variables.
Answer 94: In K&R style functions, the parameter declarations come immediately before the first curly brace.
That means that the declaration:
int sum(i1, i2, i3)
{
declares three parameters of default (int) type. Anything after that is declared as a local variable.
In particular
int sum(i1, i2, i3)
{
int i1; /* Local variable, not parameter */
int i2; /* Local variable, not parameter */
int i3; /* Local variable, not parameter */
The result is instead of summing three parameters, the program adds three uninitialized local variables. No wonder we get a strange result.
Answer 95: The problem is the statement:
24 sscanf(line, "%c %d", oper, value);
The sscanf function takes pointers as its arguments. (Remember C doesn't check arguments for the correct type.) In this case, we gave sscanf a character and an integer. We should have given it a pointer to a character and a pointer to an integer:
24 sscanf(line, "%c %d", &oper, &value);
Answer 96: The program use raw I/O to do its work (using the read and write system calls). This program does one raw read and raw write for each character. Operating calls are expensive, and this program uses 2 (one read and one write) per byte copied.
To speed up the program, cut down on the operating system calls. This can be done two ways:
Use the buffered I/O system by making the input and output fstreams instead of file descriptors.
Read and write more than one character at a time.
Answer 97: The problem is the statement:
for (index = 0; string[index] != '\0'; ++index)
/* do nothing */
return (index);
There is no semicolon after the /* do nothing */ statement.
The return is part of the for statement. The code should look like this after it is indented properly:
for (index = 0; string[index] != '\0'; ++index)
/* do nothing */
return (index);
From this code section we can see that the first time through, the for loop index will be zero and the return taken. That's why all the strings are of zero length.
What the programmer wanted was:
for (index = 0; string[index] != '\0'; ++index)
/* do nothing */;
return (index);
Answer 98: The problem is that class is allocated not by the C++ new operator, but instead uses the old style C malloc operator. This creates the space for the class without calling the constructor.
Then just to add insult to injury, memset is called to zero the class.
result =
(struct info *)malloc(sizeof(struct info));
memset(result, '\0', sizeof(result));
What the programmer should have written is:
result = new info;
| Note |
The author first found this problem in a large library he was trying to debug. Because of the large size of the library and the complexity of the mess, it took him a week to find the location of the malloc. |
out_file << ch;
does not send a character to the output. Regardless of its name, the ch variable is of type integer. The result is that the integer is printed to the output. That's why the output file is full of integers.
This is the one case in which C++'s automatic type detection of output parameters gets in your way. The old C printf statement would handle things correctly like:
printf("%c", ch);
But with C++ you must cast to get the correct results in this case:
out_file << static_cast<char>(ch);
Answer 100: The program outputs:
First: second Second: second
The problem is that the readdir returns a pointer to static data. This data is owned by readdir and overwritten by subsequent calls.
So what happens is this: We call scan_dir and set first_ptr to point to the string first. That's what we want, but the array containing the name is static and when we call readdir again, it uses the same buffer to store the name second. So now first_ptr points to second, which is the cause of our trouble.
Answer 101: In the base class destructor, we call the function clear.
This function calls a pure virtual function, delete_data.
During destruction, the derived class gets deleted first. When the derived class goes, so does the definition of delete_data. Next, the base class destructor is called. In this case, our list class indirectly calls delete_data, which is pure virtual. Because there is no derived class, the runtime system causes the program to abort.
Avoidance 37: Do not call pure virtual functions from a constructor or destructor of an abstract class.
Answer 102: I expect the results to be:
First 1
First 1
First 1
Second 1
Second 2
Second 3
but the results are:
First 0
First 0
First 0
Second 0
Second 1
Second 2
The problem is the statement:
return (i++);
Now I knew that this added one to i and returned. The problem is that i++ is the value of i before the increment. So what the statement really does is:
Save the value of i.
Increment i.
Return the saved value.
So the lines:
i = 1;
return (i++);
cause a 1 to be returned, not a 2 as one might expect.
Avoidance 38: Put ++ and - on lines by themselves.
Answer 103: The problem is that on some systems, longs must align on a four-byte boundary. So let's take a look at our structure:
struct end_block_struct
{
unsigned long int next_512_pos; // [0123]
unsigned char next_8k_pos1; // [4]
unsigned char next_8k_pos2; // [5]
unsigned long int prev_251_pos; // [6789]
6 is not divisible by 4, so the compiler adds two padding bytes to make it jump to 8. So what we really have is:
struct end_block_struct
{
unsigned long int next_512_pos; // [0123]
unsigned char next_8k_pos1; // [4]
unsigned char next_8k_pos2; // [5]
unsigned char pad1, pad2; // [67]
unsigned long int prev_251_pos; // [89 10 11]
This is not what's indented.
Avoidance 39: Put statements like
assert(sizeof(end_block_struct) == 16);
in your code to catch compilers that cause this problem.
Another avoidance is to make every member of the structure a byte and assemble the short and long ints yourself. This is more work, however.
Answer 104: The zip code 44101 is too large for MS-DOS's 16-bit integer. The largest number a 16-bit integer can hold is 32,767. The result is that the number overflows into the sign bit, and things go wrong.
| Note |
Win32 systems use 32-bit integers, so this problem does not occur on the current versions of Microsoft Windows. |
Answer 105: The ABORT macro is expanded into two statements. So the result of the if statement is:
if (value < 0)
std::cerr << "Illegal root" << std::endl;exit (8);
or properly indented:
if (value < 0)
std::cerr << "Illegal root" << std::endl;
exit (8);
From this output it's easy to see why we always exit.
Avoidance 40: Use inline functions instead of multistatement macros.
inline void ABORT(const char msg[]) {
std::cerr << msg << std::endl;
exit(8);
}
Avoidance 41: If you must use multistatement macros, enclose them in curly braces:
#define ABORT(msg) \
{std::cerr << msg << std::endl;exit(8);}
Answer 106: The problem is the statement:
char prev_ch = '\0';
Because prev_ch is an automatic variable, this variable is created and initialized at the beginning of each loop. This means for the first if the variable prev_ch will always hold '\0' and we'll never match double letters.
Answer 107: This program makes the big mistake of using floating-point for money. Floating-point numbers may not be exact. When adding up a lot of floating-point numbers, some errors may creep in.
The solution is to change the program to store money not in fractional dollars but as an integer number of cents.
Avoidance 42: Don't use floating-point for money or anything else you want represented exactly.
Answer 108: The printf call prints whatever string you give it. If you add 1 to a character string, you get the string minus the first character.
So:
printf("-xxx") prints -xxx
printf("-xxx" + 1) prints xxx
The expression ((flags & 0x4) != 0) returns a 0 or 1 depending on whether the bit is set.
The programmer is printing -word if the bit is set ("-word" + 0). The output is word if it is clear ("-word" + 1).
| Note |
If you are going to be this clever in your code, comment it to tell the maintenance programmers how smart you are. |
Answer 109: The problem is the operator = function. It's defined as:
trouble operator = (const trouble &i_trouble)
{
std::cout << "= operator called\n";
data = i_trouble.data;
return (*this);
}
The return value of this function is the class trouble. But there's a problem. Because the function does not return a reference, a copy of the variable has to be made. That means that the copy constructor has to be called. This calls the operator = function, which does the return, calling the copy constructor and so on.
The solution is to have the operator = function return a reference to the class:
trouble& operator = (const trouble &i_trouble)
Answer 110: The initialization of log_file can call new. Of course, our new new uses the log_file, so the log_file may be used before it gets constructed, confusing the whole mess.
Avoidance 43: Don't redefine the global new and delete unless you know what you are doing. Really know what you are doing. Even then don't do it.
Answer 111: The problem is that the initialization order of global variable is not guaranteed. In this case, a_var assumes that std::cout is initialized. That may not be the case.
Let's assume the worse and assume that the initialization order is a_var, std::cout. In that case, a_var is created. The constructor is called and output a message to std::cout. Because std::cout has not been created yet, things get very confused and the program crashes.
Answer 112: The problem is that MAX is defined to be literally the text "=10" That means that
for (counter =MAX; counter > 0; --counter)
expands to
for (counter ==10; counter > 0; --counter)
This does not initialize the counter (it merely compares counter to 10 and throws the result). Because the counter is not initialized we get a random number of greetings.
| Note |
The GNU preprocessor sticks spaces around macro expansions so that the GNU version of the expansions:
for (counter = =10 ; counter > 0; --counter)
It's unfortunate that the good GNU technology is robbing us of the opportunity of debugging strangely failing programs. |
Answer 113: The space after the name DOUBLE makes this macro a simple text replacement macro. Thus,
#define DOUBLE (value) ((value) + (value))
causes DOUBLE to be replaced with:
(value) ((value) + (value))
Literally!
This means that the line
std::cout << "Twice " << counter << " is " <<
DOUBLE(counter) << '\n';
looks like:
std::cout << "Twice " << counter << " is " <<
(value) ((value) + (value)) (counter) << '\n';
(Indentation added.)
Solution: Define DOUBLE as
#define DOUBLE(value) ((value) + (value))
Avoidance 44: Use inline functions instead of parameterized macros whenever possible. Example:
inline DOUBLE(const int value) {
return (value + value);
}
Answer 114: The problem is that the optimizer feels free to rewrite the code. Some optimizers will stick variables in registers to make the code go faster. For example, one optimized version of this program looks like:
1 /************************************************
2 * sum -- Sum the sine of the numbers from 0 to *
3 * 0X3FFFFFFF. Actually we don't care *
4 * about the answer, all we're trying to *
5 * do is create some sort of compute *
6 * bound job so that the status_monitor *
7 * can be demonstrated. *
8 ************************************************/
9 /* --- After the optimizer --- */
10 /* --- gets through with it --- */
11 static void sum(void)
12 {
13 static double sum = 0; /* Sum so far */
14 register int reg_counter = counter;
15
16 for (reg_counter = 0;
17 reg_counter < 0x3FFFFFF; ++reg_counter)
18 {
19 sum += sin(double(reg_counter));
20 }
21 printf("Total %f\n", sum);
22 counter = reg_counter;
23 exit (0);
24 }
From this, we can see that counter is updated only after the program finishes. If we try to examine it at any time in the other thread we die.
The solution it to declare the variable volatile:
volatile int counter;
Then the compiler will make no assumptions about what it can do about it regarding optimization, and will generate code that keeps counter is kept up-to-date.
Answer 115: I am trying to always make sure I delete the variable data before I overwrite it so I don't have a memory leak. I even delete it in the following code:
34 // Copy constructor
35 v_string(const v_string &old)
36 {
37 if (data != NULL)
38 {
39 delete[] data;
40 data = NULL;
41 }
42 data = strdup(old.data);
43 }
This is the copy constructor. The first thing it does is to see if data has anything in it and, if so, delete it. But what could data possibly have in it? We just created the class and haven't initialized it yet. So we are deleting a random pointer and as a result, crashing. Properly written our copy constructor should be:
34 // Copy constructor
35 v_string(const v_string &old):
36 data(strdup(old.data))
37 {}
|
|