Changed virtual function prototype results in correct function not being called
This one wasn’t a bug that landed immediately from my change, but happened when a subsequent code branch was integrated. I’m going to claim this as my bug as it could have been prevented (or at least made more obvious) with some extra though.
The problem:
We have a virtual function, something like
class Base {
public:
virtual void Foo(int some_param) { }
};
Which I modified a few weeks back by adding an extra parameter, so foo() became
virtual void foo(int some_param, WhatsitType some_other_param) { }
and I added the same parameter to every one of the classes that inherited from class Base. There were quite a few.
Sometime later, code was integrated from another branch where the old prototype was still in use, including something like this:
class NewInheritor : public Base {
public:
virtual void Foo(int some_param) { // oh noes - it's the old prototype!
DoAmazingThings();
}
};
The new code compiled without warning, but at runtime, NewInheritor::Foo() wasn’t being called, so the amazing things were failing to be done. Debugging this was somewhat inefficient — the process for calling Foo() methods includes a number of steps, and I failed to notice the missing parameter for quite a while.
The fix:
Update NewInheritor::Foo() to match the parent class.
Preventing these in future:
Remember that virtual inheritance is a horribly fragile beast.
One thing that may have worked is to make Foo() pure (compile error) — or undefined (link error) — in the base class (i.e. void Foo() = 0;). This isn’t a great match in this case as inheriting classes don’t need to have their own implementation of Foo() to be valid, and requiring it makes for some additional tedious boilerplate code in every inheritor (of which there are many).
The override keyword will help detect this at compile time, but it relies on the implementer of NewInheritor to have used it :\ The action here is to recommend that people use override on every virtual function override.
In this case, what I do know about this code is that it is never valid to call Foo() on a class that doesn’t have its own implementation. We can put an assert() in Base::Foo() and let the writers of inheriting types (or me when next I get to debug one of these) have a more obvious clue about what’s happening. Adequately functional.
memcpy() into an uninitialized pointer variable
The problem:
I had changed the type of a variable from being a static array to being a pointer, to match a change I’d made to a particular function. What I hadn’t paid attention to was that this array was being used as the destination address for a call to memcpy(). An uninitialized pointer on the stack is not a good place to try to copy things.
The call to memcpy() did not cause the program to crash. The program did crash shortly thereafter, though, which distracted me for a short while from the cause.
The fix:
Explicitly allocate some space for the copy.
Preventing in the future:
The pointer on the stack had not been set. A null pointer would have caused the crash to happen at the call to memcpy(), making the cause more obvious.
Bad loop counting
The problem:
int max = m_Max;
int i = 0;
for( ; i < max; ++max )
if( a[i] == bar )
break;
++max? Yeesh.
m_Max is initialized to zero, which means that this was a no-op rather than an infinite loop.
The fix:
Increment the counter, not the upper bound.
Preventing in the future:
Don’t be a goose.
Failing to store updated state
The problem:
Immediately following the code above, in the case where we failed to find the desired bar in the array, increase the size of the array. And then fail to store the new max size into m_Max.
I even got this wrong twice: my first attempt was to replace a use of max+1 with ++max.
The fix:
Write the necessary state back to the object: ++m_Max.
Preventing in the future:
Pay attention to where the state should go. Don’t be a goose.
