for (i=0; i<numrows; i++)
for (j=0; j<numcols; j++);
pixels++;
Commentary: Caused by a stray ";" on line 2. Accidental bugs are often caused by stray characters, etc. While "minor" in their fix, they can be the devil to find!
Note: if used correctly, a "prettyprinter" or auto-indenter would help you spot this one.
int minval(int *A, int n) {
int currmin;
for (int i=0; i<n; i++)
if (A[i] < currmin)
currmin = A[i];
return currmin;
}
Commentary: Since currmin was never initialized, it could
easily start out as the minimum value. Some compilers spot
no-initialization errors. Note that an improper initialization,
while rarer, is even harder to spot than a missing one!
int minval(int *A, int n) {
int currmin = MAXINT;
for (int i=0; i<n; i++)
if (A[i] > currmin)
currmin = A[i];
return currmin;
}
Commentary: Here, the ">" on line 5 should be "<". Even people who are not normally dyslexic are subject to these types of errors.
switch (i) {
case 1:
do_something(1); break;
case 2:
do_something(2); break;
case 3:
do_something(1); break;
case 4:
do_something(4); break;
default:
break;
}
Commentary: The cases were generated by copying case 1. Under case 3, the values were not changed as appropriate for the case. Code reuse is good -- but this form of code copying has its dangers!
if (foo = 5)
foo == 7;
Commentary: Two bugs in one. These are usually caused by accident rather than misunderstanding. The "=" of line 1 should probably be "==" (this one will always evaluate to true), while the "==" of line 2 should almost certainly be "=" (it has no effect). A syntactic weakness in C/C++, neither of these statements is syntactically wrong. Many compilers will warn you about both of these.
int i = 5;
int j;
int foo(int j) {
for (i=0; i<j; i++) do_nothing();
return j;
}
void ineedj(void) {
cout << "j is " << j << "\n";
}
main() {
int j;
j = foo(i);
ineedj();
}
Commentary: This illustrates some fun with global/local variables. In
function foo, j is local and i
is global. Since i is being used as a loop variable, this
is almost certainly wrong. Making j local here may or may
not be logically correct, but it is certainly stylistically incorrect
since the semantic meaning of j is being used in two
distinct ways (once as a global, once as a local, which by definition
must be inconsistent). In main, j is local. So,
when it gets set by the call to foo, the global value is
not being set. So, ineedj is out of luck -- the value is
still undefined.
Moral: If the variable is global, never use that name for anything else.
// random returns a random (positive) integer.
// Random returns a random integer in the range 0 to n-1.
#define Random(n) random()%n
val = Random(j-i+1);
Commentary: The result when j=7 and i=6 is (sometimes) -5. Why? You might think there is a bug in the system routine random. But actually, the error is in the macro definition. The expansion becomes:
random()%j-i+1
Since % binds more tightly than + or -, we get random()%7 first. If random() gives a multiple of 7, then random()%7 = 0, 0-6+1 = -5.
Secondary moral: Don't be too quick to blame the compiler or the libraries! In this example, the temptation is to believe that random() (a system function) gives a bad value, or that "%" itself is buggy.
char* string1 = "Hello";
char* string2 = "World";
if (string1 == string2)
do_something();
Unless == has been explicitly overloaded, this is the wrong way to
compare the value of two strings. What is really being compared
here are the values of the two pointers. To compare the values of the
strings (which is much more likely to be what is wanted), use
strcmp.
The error is classified as a "model" error, since the user
may well have a wrong model about what == can do.
// Return pointer to the node storing "val" if any; NULL otherwise
void find(listnode **curr, val) {
while (*curr != NULL)
if (*curr->val == val) return;
else
*curr = *curr->next;
}
The *curr-> construct should be
(*curr)-> (in two places). The reason is that
-> binds more tightly than * in operator precedence,
but the user probably did not realize this. Again, this is classified
as a "model" error since the user probably believes
(implicitly) that the operator precedence goes the other way. Of
course, you could avoid the problem entirely in this particular example
by using pass-by-reference in C++.
char string1[10] = "Hello";
char string2[10];
strcpy(string1, string2);
Commentary: Oops -- strcpy's first parameter is the
destination, and its second parameter is the source, not the other way
around! Whether this is a dyslexic error or a model error of course
depends on what the programmer believes the proper order to be.
char string[4];
for (i=0; i<4; i++)
getchar(&string[i]);
cout << string;
Commentary: We didn't explicitly terminate the string, so there is no
telling where the string will end. cout will keep going
until it sees a terminator, regardless of how long it takes.
int i;
char string[5] = "hello";
int j;
Commentary: Oops! "hello" has a sixth character, the terminator. We just corrupted one of the surrounding variables on the stack.
char* ptr;
cin >> ptr;
Commentary: ptr has no storage of its own, its just a
pointer. We are writing to space that ptr happens to be
pointing at. If you are lucky, the program will crash
immediately since you are writing to an illegal memory location. If
you are unlucky, the memory location is legal and the program
won't crash until much later!
int i;
int array[5];
int j;
for (i=0; i<=5; i++)
cin >> array[i];
Commentary: We meant to go from 1 to 4, not one to 5. Off-by-one errors are common, and occur in many ways. This one happens to be particularly brutal since it results in a memory error (corrupting one of the surrounding variables).
// Delete the node following the one that ptr is pointing at.
void del_link(lnode* ptr) {
ptr->next = ptr->next->next;
}
Commentary: Here are three errors. First, if ptr
happens to be NULL, we can't follow to its
next field. Second, if ptr points to a node,
but that node is the last on its list, then we can't go to
ptr->next->next. Third, we just dropped the space
for the deleted node into the bit-bucket: OK in JAVA or LISP, but not
in C or C++!
char *initialize() {
char string[80];
char* ptr = string;
return ptr;
}
main() {
char *myval = initialize();
do_something_with(myval);
}
Commentary: Since string is a local variable, its space is
lost after returning from initialize. This space will be
reused by the next function to be called. Eventual disaster!
char* assign() {
return "hello world!";
}
main() {
char *ptr = assign();
}
Commentary: Essentially the same bug as the previous example. The
space for the string is local to assign, and gets returned
after leaving that function.
// Insert a value into an ordered linked list
void insert(lnode*& curr, int val) {
if (curr == NULL)
curr = new lnode(val, NULL);
else if (lnode->val > val)
curr = new lnode(val, curr->next);
else {
curr = curr->next;
insert(curr, val);
}
}
Commentary: The goal here is to change the value of the pointer passed
in when we do the insert to the list. The assignment curr =
curr->next is in error because this changes its alias as well
(curr is passed by reference). Instead, the recursive call
should read insert(curr->next, val);, effectively
working on a local variable (curr->next) without then
modifying the current recursion variable (curr).
main() {
Record city;
lnode *list = NULL;
while (data_to_read()) {
Readin_data(&city);
insert(&city, &list);
}
}
void insert(Record*& city, lnode*& list) {
lnode* ptr = new lnode;
ptr->next = list;
list = ptr;
prt->data = city;
}
Commentary: Record city is being used for all of the data
fields of all lnodes created by insert. Of course, there
really is only one copy of city, so all of the lnodes have
the same information! insert needs to create a
Record (using new) for each lnode.