Code:
// In the header file:
std::vector<ToolIdleTimeInfo*> m_vToolIdleTime;
// In the cpp:
ToolIdleTimeInfo *toolState = new ToolIdleTimeInfo;
FILE* file = NULL;
if( (file = fopen((LPCSTR)tchFilePath, "r")) != NULL )
{
while(!feof(file))
{
if(::fread(toolState, sizeof(ToolIdleTimeInfo), 1, file))
m_vToolIdleTime.push_back(toolState);
}
fclose(file);
m_vToolIdleTime.begin();
}
This code has been in our codebase for three months without being noticed. Or, quite clearly, ever tested. The programmer that wrote it established a track record of cluelessness before getting himself fired, and today I decided to proactively code review some of his work. The funny thing about this is that it was the last line that caught my attention, which does nothing--it gets a pointer to the head of the m_vToolIdleTime vector, then throws it away because he doesn't do anything with it. I'm sure he thought he was changing an internal cursor to the vector or something. Normally this would make me ask "why?" but with this programmer I've learned that trying to answer that question just causes me to keen piteously and make sobbing noises.
It wasn't until I went back and examined the loop above that line that I stabbed my brain out.
What this is SUPPOSED to do, is read some data from a file, and create a new ToolIdleTimeInfo structure for each element. We push these into a vector. At some point we go back and calculate average tool utilization, which is a huge metric in the semiconductor industry. (This is a US$1.2M machine. It is understandable if management wants it to actually be
in use for 90%+ of its uptime.)
What it actually DOES, however, is:
Create one ToolIdleTimeInfo structure, and get its pointer (which, as you may notice, is never tested to see if new succeeded). Load a record from file, write it to the memory pointed at by our pointer, and push the pointer into our vector. So far, so good. Now, load another record,
and write it to the same location in memory. He failed to get another new pointer. This overwrites the previous record. Now, push
the same pointer into the vector
again.
End result, if you have n items in the data file, you will have a vector with n copies of the same pointer, all pointing to the piece of memory we started out with and which now contains the last record in the file.
Oh, and another missed test: he never truncates this vector, neither in the load, nor anywhere else at runtime. At runtime, he extends the vector correctly. Which means this program leaks about 100kB/hr. Given that this machine is expected to have uptimes of 2-6
months, they'd better have half a gigabyte of memory free just for this leaking vector.
Did I mention the customer signed off on this code, and we are now in "maintenance" mode?
This is shipped code.