Function to erase original element but also linked

Question

I have a list of pointers to objects where some of the objects contain a pointer to another object. When I remove an object in the list which has a linked object I need to remove them BOTH. I also want to position the next iterator at a convenient position which will usually be just after the object in the list which is to be deleted. Or if the linked item is next it will have to be just after the linked item.

There are peripheral issues such as why not using smart pointers etc. But this is just a simple demo - so please ignore lifecycle issues. I am looking for comments on the RemoveAllAssociated function.

   #include <iostream>
   #include <list>
   #include <algorithm>

   using namespace std;


   class A
   {
   public:
      A(int timestamp) : m_timestamp(timestamp), m_red(false), m_linkedmsg(0) {}
      virtual ~A() {}

      int gettimestamp() const { return m_timestamp; }

      A* getLinkedMsg() const { return m_linkedmsg; }
      void setLinkedMsg(A* linked) { m_linkedmsg = linked; }

      bool IsRed() const { return m_red; }
      void setRed(bool col = true) { m_red = col; }

   protected:
      int m_timestamp;
      bool m_red;
      A* m_linkedmsg;
   };

   class B : public A
   {
   public:
      B(int timestamp) : A(timestamp) { setRed(true); }
      virtual ~B() {}

   };

   //erase item and linked iterator if available and return next iterator
   std::list<A*>::iterator RemoveAllAssociated(std::list<A*>& thelist, A* item) {
      std::list<A*>::iterator it = find(thelist.begin(), thelist.end(), item);
      if(it != thelist.end()) {
         cout << "found original item in list with timestamp=" << (*it)->gettimestamp() << "\n";
         //we now want to delete linked as well as original
         //first delete linked
         A* linked = (*it)->getLinkedMsg();
         std::list<A*>::iterator found_it = find(thelist.begin(), thelist.end(), linked);
         if(found_it != thelist.end()) {
            cout << "found linked item with timestamp=" << (*found_it)->gettimestamp() << "\n";
            delete *found_it;
            std::list<A*>::iterator after_linked_it = thelist.erase(found_it);
         }

         //now erase item in list
         //Need to return next iterator.  Usually will be just after item 
         //check a valid iterator returned
         //We want iterator to be positioned either just after 
         delete *it;  //or could have done: delete item;
         return thelist.erase(it);
      } else
         cout << "original item not found in list\n";   

      return thelist.end();
   }

   int main() {

      A* a1 = new B(0);
      A* a2 = new B(1);
      A* a3 = new B(2);
      A* a4 = new B(3);
      A* a5 = new B(4);
      A* a6 = new B(5);

      a2->setLinkedMsg(a1);

      //put them in a list
      std::list<A*> alist;
      alist.push_back(a5);
      alist.push_back(a6);
      alist.push_back(a1);
      alist.push_back(a2);
      alist.push_back(a3);
      alist.push_back(a4);

      std::list<A*>::iterator it = RemoveAllAssociated(alist, a2);
      if(it != alist.end())
         cout << "next item in list is " << (*it)->gettimestamp() << endl;

      //print out what is left to check correct items erased
      for(it = alist.begin(); it != alist.end(); ++it) {
         cout << "address=" << *it << " ts=" << (*it)->gettimestamp() << endl;
      }

      //cleanup
      it = alist.begin();
      while(it != alist.end()) { 
         delete *it;
         it = alist.erase(it);
      }

      return 0;
   }

Answer 1

OK. This is NOT C++ code. This is C that happens to use a few C++ features like std::list. I would call this "C with classes".

What you have failed to do is define the ownership of the pointers. Without ownership calling delete is a pain (because you don't know if calling delete is a good idea because you don't know the ownership).

There is no point in reviewing further until you define ownership semantics.
Who owns the pointer.

• Is it shared ownership.
• Is it exclusive ownership.
• Are there cycles?

This looks really awful:

  A* a1 = new B(0);
  A* a2 = new B(1);
  A* a3 = new B(2);
  A* a4 = new B(3);
  A* a5 = new B(4);
  A* a6 = new B(5);

You have 6 RAW pointers. What happens if the constructor to B(3) throws an exception. Then you have just leaked a1/a2/a3. In modern C++ it is very rare to see RAW pointers. Pointers are usually contained inside an object (usually a smart pointer).

Now you are passing the pointers to a list.

  //put them in a list
  std::list<A*> alist;
  alist.push_back(a5);
  alist.push_back(a6);
  alist.push_back(a1);
  alist.push_back(a2);
  alist.push_back(a3);
  alist.push_back(a4);

So now you have two people pointing at each object. So who is supposed to destroy them? You have not defined the ownership semantics thus we do not know who is responsible for deleting the pointers. Even worse if they are deleted via one method the other pointer points at an invalid location (as there is no feedback) between the pointers.

boost::ptr_list would hold and take ownership of the pointers very nicely.

Cleanup outside a destructor!!!

  //cleanup
  it = alist.begin();
  while(it != alist.end()) { 
     delete *it;
     it = alist.erase(it);
  }

This is not exception safe. You need to use RAII to make sure you can never leak resources even in the presence of pointers.