Gotcha #83: Failure to Distinguish Aggregation and Acquaintance

class Employee { 
 public:
   virtual ~Employee();
   void setRole( Role *newRole );
   const Role *getRole() const;
   // . . .
 private:
   Role *role_;
   // . . .
};

It's not clear from the above interface whether an Employee object owns its Role or simply refers to a Role that may be shared by other Employee objects. The problems occur when a user of the Employee class makes an assumption about ownership that differs from what the designer of the Employee class implemented:

Employee *e1 = getMeAnEmployee(); 
Employee *e2 = getMeAnEmployee();
Role *r = getMeSomethingToDo();
e1->setRole( r );
e2->setRole( r ); // bug #1!
delete r; // bug #2!

In the case where the designer of the Employee class has decided that the Employee owns its Role, the line marked bug #1 will result in two Employee objects aliasing the same Role object. At a minimum, a double deletion of the Role object will occur when e2 and e1 are deleted and their destructors each delete the unintentionally shared Role object.

The line marked bug #2 is more problematic. Here, the user of the Employee class is assuming that setRole makes a copy of its Role argument and the allocated Role object must be cleaned up. If this is not the case, both e1 and e2 will contain dangling pointers.

An experienced developer might look at the implementation of the setRole function for a clue and be confronted with one of the following implementations:

void Employee::setRole( Role *newRole ) // version #1 
   { role_ = newRole; }

void Employee::setRole( Role *newRole ) { // version #2
   delete role_;
   role_ = newRole;
}

void Employee::setRole( Role *newRole ) { // version #3
   delete role_;
   role_ = newRole->clone();
}

Version #1 of the setRole function indicates that the Employee object doesn't own its Role object, since no attempt is made to clean up the existing Role object before setting a pointer to the new one. (We're making the assumption that this is a reflection of the intent of the designer of the Employee class and not a simple bug.)

Version #2 indicates that the Employee owns its Role object and is taking over ownership of the Role referred to by the argument. Version #3 also indicates that the Employee object owns its Role. However, it makes a copy of its Role argument rather than simply taking control of it. Note that it would be better, in the case of version #3, to declare the argument to be const Role * rather than Role *. Cloning is invariably a const operation, since it doesn't modify its object but simply makes a copy of it. It would also be unusual for a shared Role object, such as version #1 implies, to be passed as a pointer to non-const.

However, users of an abstract data type don't generally have access to its implementation, since such access would tend to defeat data hiding and promote dependency on a particular implementation. For example, the fact that version #1 of setRole above doesn't delete the existing Role does not necessarily indicate that the designer of the Employee class intended to share the Role object; it might simply have been a bug. However, after a significant amount of client code has made the assumption that Roles are shared, it's no longer a bug. It's a feature.

Since it's not possible to specify ownership issues directly in C++ language, we have to resort to naming conventions, formal argument types, and (yes, in this case) comments:

class Employee { 
 public:
   virtual ~Employee();
   void adoptRole( Role *newRole ); // take ownership
   void shareRole( const Role *sharedRole ); // does not own
   void copyRole( const Role *roleToCopy ); // set role to clone
   const Role *getRole() const;
   // . . .
};

The names adoptRole, shareRole, and copyRole are unusual enough to encourage users of the Employee class to read the comments. If the comments are short and clear, they may even be maintained. (See Gotcha #1.)

One common source of ownership miscommunication occurs with containers of pointers. Consider a list of pointers type:

gotcha83/ptrlist.h

template <class T> class PtrList; 
template <> class PtrList<void> {
   // . . .
};
template <class T>
class PtrList : private PtrList<void> {
 public:
   PtrList();
   ~PtrList();
   void append( T *newElem );
   // . . .
};

The problem is, once again, the likelihood of miscommunication between the designer and users of the container:

PtrList<Employee> staff; 
staff.append( new Techie );

In the code above, the user of PtrList is probably assuming that the container is taking ownership of the object referred to by the argument to append—that is, that the PtrList destructor is going to delete all the objects its pointer elements refer to. If the container doesn't perform such a cleanup, there will be a memory leak. The author of the code below makes a different assumption:

PtrList<Employee> management; 
Manager theBoss;
management.append( &theBoss );

In this case, the user of the PtrList container is assuming that the container will not attempt to delete the objects to which its elements refer. If this isn't the case, the PtrList will attempt to delete unallocated storage.

The best way to avoid miscommunication of ownership in containers is to use standard containers. Because these containers are part of the C++ standard, all experienced C++ programmers have shared understanding of their behavior. For pointer elements, the standard containers will clean up the pointers but not the objects to which they refer:

std::list<Employee *> management; 
Manager theBoss;
management.push_back( &theBoss ); // correct

In the case where we'd like the container to clean up the objects to which its elements refer, we have a couple of choices. The most straightforward approach is to perform the cleanup manually:

template <class Container> 
void releaseElems( Container &c ) {
   typedef typename Container::iterator I;
   for( I i = c.begin(); i != c.end(); ++i )
       delete *i;
}
// . . .
std::list<Employee *> staff;
staff.push_back( new Techie );
// . . .
releaseElems( staff ); // clean up

Unfortunately, manual cleanup code is easily forgotten, often removed or misplaced during maintenance, and generally fragile in the presence of exceptions. Often, a better choice is to use a smart pointer element in place of a raw pointer. (Note that the standard auto_ptr template should not be used as a container element, due to the inappropriate semantics of its copy operations. See Gotcha #68.) A simple example of such a pointer might look like this:

gotcha83/cptr.h

template <class T> 
class Cptr {
 public:
   Cptr( T *p ) : p_( p ), c_( new long( 1 ) ) {}
   ~Cptr() { if( !--*c_ ) { delete c_; delete p_; } }
   Cptr( const Cptr &init )
       : p_( init.p_ ), c_( init.c_ ) { ++*c_; }
   Cptr &operator =( const Cptr &rhs ) {
       if( this != &rhs ) {
           if( !--*c_ ) { delete c_; delete p_; }
           p_ = rhs.p_;
           ++*(c_ = rhs.c_);
       }
       return *this;
   }
   T &operator *() const
       { return *p_; }
   T *operator ->() const
       { return p_; }
 private:
   T *p_;
   long *c_;
};

The container is instantiated to contain smart pointers rather than raw pointers (see Gotcha #24). When the container deletes its elements, the smart pointer semantics clean up the objects to which they refer:

std::vector< Cptr<Employee> > staff; 
staff.push_back( new Techie );
staff.push_back( new Temp );
staff.push_back( new Consultant );
// no explicit cleanup necessary . . .

The utility of this smart pointer extends to more complex cases as well:

std::list< Cptr<Employee> > expendable; 
expendable.push_back( staff[2] );
expendable.push_back( new Temp );
expendable.push_back( staff[1] );

When the expendable container goes out of scope, it will correctly delete its second, Temp element and decrement the reference counts of its first and third elements, which it shares with the staff container. When staff goes out of scope, it will delete all three of its elements.