C++ SmartPointers leak on self assign?

Question

Hello, i have small problem understanding why my smart pointer class is leaking on self assing. If i do something like this

SmartPtr sp1(new CSine());//CSine is a class that implements IFunction iterface
sp1=sp1;

my colleagues told me that my smart pointer leaks. I added some log messages in my smart pointer to track what is going on and a test and reported this:

SmartPtr sp1(new CSine());
->CSine constructor
->RefCounter increment 0->1
->RefCounter constructor
->SmartPtr constructor

sp1=sp1;
->checks if this.RefCounter == to parameter.RefCounter, if true returns the smart pointer unmodified else modifies the object and returns it with the new values; in this case it returns true and returns the object unchanged.

at the end
->SmartPtr destructor
->RefCounter decrement 1->0
->RefCounter destructor
->CSine destructor

i can't understand why they consider that my smart pointer leaks...any ideas? Thank you in advance!

class SmartPtr
{
private:
    RefCounter* refCnt;
    void Clear()
    {
     if(!isNull() && refCnt->Decr() == 0)
      delete refCnt;
     refCnt = 0;
    };
public:
    explicit SmartPtr();
    explicit SmartPtr(IFunction *pt):refCnt(new RefCounter(pt)){};
    SmartPtr(SmartPtr& other)
    {
     refCnt = other.refCnt;
     if (!isNull())
      refCnt->Incr();
    };
    virtual ~SmartPtr(void){Clear();};

    SmartPtr& operator=(SmartPtr& other)
    {
     if(other.refCnt != refCnt)
     {
      if(!rVar.isNull())
       other.refCnt->Incr();
      Clear();
      refCnt = other.refCnt;
     }
     return *this;
    };

    SmartPtr& operator=(IFunction* _p)
    {

     if(!isNull())
     {
      Clear();
     }
     refCnt = new RefCounter(fct);
     return *this;
    };

    IFunction* operator->();
    const IFunction* operator->() const;
    IFunction& operator*();
    const IFunction& operator*() const;
    bool isNull() const { return refCnt == 0; };

    inline bool operator==(const int _number) const;
    inline bool operator!=(const int _number) const;
    inline bool operator==(IFunction* _other) const;
    inline bool operator!=(IFunction* _other) const;
    inline bool operator==(SmartPtr& _other) const;
    inline bool operator!=(SmartPtr& _other) const;
};

class RefCounter
{
    friend class SmartPtr;
private:
    IFunction* p;
    unsigned c;

    explicit RefCounter(IFunction* _p):c(0),p(_p)
    {
     if(_p != NULL)
      Incr();
     cout<<"RefCounter constructor."<<endl;
    }
    virtual ~RefCounter(void)
    { 
     cout<<"RefCounter destructor."<<endl;
     if(c == 0)
      delete p; 
    }
    unsigned  Incr()
    {
     ++c;
     cout<<"RefCounter increment count:"<<c-1<<" to "<<c<<endl;
     return c; 
    }
    unsigned  Decr()
    {
     if(c!=0)
     {
      --c;
      cout<<"RefCounter decrement count:"<<c+1<<" to "<<c<<endl;
      return c;
     }
     else
      return 0;
    }
};

Answer 1

SmartPtr& operator=(SmartPtr& other)
    {
        if(rVar.refCnt != refCnt)

should be:

    if ( this != & other )

Answer 2

You might want to look at the following quote from A Proposal to Add General Purpose Smart Pointers to the Library Technical Report:

The Boost developers found a shared-ownership smart pointer exceedingly difficult to implement correctly. Others have made the same observation. For example, Scott Meyers [Meyers01] says:

"The STL itself contains no reference-counting smart pointer, and writing a good one - one that works correctly all the time - is tricky enough that you don't want to do it unless you have to. I published the code for a reference-counting smart pointer in More Effective C++ in 1996, and despite basing it on established smart pointer implementations and submitting it to extensive pre- publication reviewing by experienced developers, a small parade of valid bug reports has trickled in for years. The number of subtle ways in which reference-counting smart pointers can fail is remarkable."

If this is homework, read about how to implement copy ctor and assignment operator using a swap() (member) function. Otherwise, do not try to write your own smart pointer. You cannot win.

Answer 3

My impression is that there is no memory leak. To be sure:

• test with valgrind or the VS-alternative
• use std::tr1::shared_ptr (if this is more than educational)

Answer 4

Your code doesn't compile, which leads me to believe that the version you posted can't be the version you colleagues are complaining about.

Answer 5

I don't see a leak either, but I think there are some other problems (other than many compiler errors - this cannot be the code you are using):

SmartPtr& operator=(SmartPtr& other)

should take the argument by const reference. You don't have to increment the reference count of other, because you can do it on the non-const left-hand side, as they will be sharing the same reference count instance.

Next, the canonical way to implement assignment for such classes is using the copy-and-swap idiom - which means you should also define a trivial swap method (which just swaps the pointers), and worry less about self-assignment :)

Answer 6

Pretty much any smart pointer will have cases where it leaks. It's just the way that it has to be if you implement it using references. There's also a million other problems plus they are slow. Since they are buggier than raw pointers there's really not much use if all you get out of it is reference counting. I have been tempted to use them for some very special purposes but they are not for general programming use. There's a reason that they are not allowed in STL containers for example.