Getting C++ Constructors Right is Vitally Important
I ran into a problem that completely reinforces my belief that coding standards are a very good thing. And I was the culprit here - but only because I missed it, not by choice. What I had was a constructor for a Dividends set - basically dates and values in a table that needs to be treated as a single unit. The copy constructor for the instance looked like this:
Dividends::Dividends( const Dividends & anOther ) { // let the '=' operator do all the heavy lifting this->operator=((Dividends &)anOther); }
and the operator=() method looked like this:
Dividends & Dividends::operator=( Dividends & anOther ) { if (this != & anOther) { boost::detail::spinlock::scoped_lock lock(mDivsMutex); mSetName = anOther.mSetName; mSecurityKey = anOther.mSecurityKey; mDivs = anOther.mDivs; mUpdateTimestamp = anOther.mUpdateTimestamp; } return *this; }
Now the trick here is to realize that the Boost spin lock is really just an atomic integer that they wrap with CAS operations. It's good, but it really needs to be initialized properly or it's not going to function well. What I'm doing in the copy constructor isn't wrong in the sense that it's logic is wrong, but the implementation is horrible.
When I saw this, it was clear to me what the bug was: the spin lock wasn't initialized properly, and the use of it in the operator=() method was locking the thread. Bad news. The solution was simple:
Dividends::Dividends( const Dividends & anOther ) : mSetName(anOther.mSetName), mSecurityKey(anOther.mSecurityKey), mDivs(anOther.mDivs), mDivsMutex(), mUpdateTimestamp(anOther.mUpdateTimestamp) { // the initialized values do everything I need }
and the advantages are clear: since we're constructing a new instance, locking the new instance isn't necessary - no one knows it's there. Also, we properly initialize the mutex, and that's everything in this problem.
Standards. Get them. Live with them.