CODEWITHSUNDEEP
c++pointersmemory-leakssmart-pointers
Sorin Antohi
Sorin Antohi

Reputation: 6135

C++ SmartPointers leak on self assign?

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;
    }
};

Upvotes: 1

Views: 1193

Answers (6)

Charles Eli Cheese
Charles Eli Cheese

Reputation: 783

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.

Upvotes: -1

JoeG
JoeG

Reputation: 13192

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.

Upvotes: 0

UncleBens
UncleBens

Reputation: 41351

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 :)

Upvotes: 1

stefaanv
stefaanv

Reputation: 14392

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)

Upvotes: 0

sbi
sbi

Reputation: 224129

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.

Upvotes: 2

anon
anon

Reputation: 

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

should be:

    if ( this != & other )

Upvotes: 3

Related Questions