CODEWITHSUNDEEP
c++shared-ptr
Vasiliy Stephanov
Vasiliy Stephanov

Reputation: 23

Problem with shared_ptr. I cannot create my struct without mistakes

Please help find a mistake in my custom shared pointer class SharedPtr<T>. I cannot find it. But code works with leaks.

I think that mistakes in reset method or in operator= method. Probably without call these methods code works. I think so because of valgrind shows no leaks and mistakes. But with calling these methods it shows mistakes, sometimes leaks

#include <iostream>
#include <algorithm>
#include <memory>

using std::cout;
using std::cin;
using std::endl;

template <typename T>
struct SharedPtr
{
    explicit SharedPtr(T *ptr = 0) : p_(ptr), c_(0)
    {
        if (p_) c_ = new size_t(1);
    }
    ~SharedPtr()
    {
        if (c_ && --*c_ == 0) 
        {
            delete p_; 
            delete c_;
        }
    }
    SharedPtr(const SharedPtr &p) : p_(p.get()), c_(p.getc()) 
    {
        if (c_) { ++*c_; }
    }
    SharedPtr& operator=(const SharedPtr &p)
    {
        p_ = p.p_;
        c_ = p.c_;
        return *this;
    }

    T* get() const { return p_;}
    size_t* getc() const { return c_; }
    size_t getcc() const { return *c_; }

    void reset(T *ptr = 0)
    {
        SharedPtr(ptr).swap(*this);
        // if (c_ && --*c_ == 0) 
        // {
        //     delete p_; 
        //     delete c_;
        // }
        // if (ptr == nullptr)
        // {
        //     c_ = nullptr;
        // }
        // p_ = ptr;
    }

    T& operator*() const { return *p_;}
    T* operator->() const { return p_;}
private:
    void swap(SharedPtr& other)
    {
        std::swap(p_, other.p_);
        size_t* tmp = c_;
        c_ = other.c_;
        other.c_ = tmp;
    }
    T * p_;
    size_t * c_;
};

int main ()
{
    SharedPtr<int> p1(new int(5));
    SharedPtr<int> p2(new int(10));
    SharedPtr<int> p5;
    p5 = p2;
    p1 = p5;
    p1.reset();
}

valgrind output:

==12777== HEAP SUMMARY:
==12777==     in use at exit: 12 bytes in 2 blocks
==12777==   total heap usage: 5 allocs, 3 frees, 72,728 bytes allocated
==12777==
==12777== LEAK SUMMARY:
==12777==    definitely lost: 12 bytes in 2 blocks
==12777==    indirectly lost: 0 bytes in 0 blocks
==12777==      possibly lost: 0 bytes in 0 blocks
==12777==    still reachable: 0 bytes in 0 blocks
==12777==         suppressed: 0 bytes in 0 blocks
==12777== Rerun with --leak-check=full to see details of leaked memory
==12777==
==12777== For lists of detected and suppressed errors, rerun with: -s
==12777== ERROR SUMMARY: 6 errors from 6 contexts (suppressed: 0 from 0)

And one of same mistakes:

==12777== Invalid read of size 8
==12777==    at 0x1093F9: SharedPtr<int>::~SharedPtr() (in /home/baseoleph/git/stepik/1_7_CPP/a.out)
==12777==    by 0x109296: main (in /home/baseoleph/git/stepik/1_7_CPP/a.out)
==12777==  Address 0x4da5d70 is 0 bytes inside a block of size 8 free'd
==12777==    at 0x483D1CF: operator delete(void*, unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==12777==    by 0x10944D: SharedPtr<int>::~SharedPtr() (in /home/baseoleph/git/stepik/1_7_CPP/a.out)
==12777==    by 0x1094DA: SharedPtr<int>::reset(int*) (in /home/baseoleph/git/stepik/1_7_CPP/a.out)
==12777==    by 0x10928A: main (in /home/baseoleph/git/stepik/1_7_CPP/a.out)
==12777==  Block was alloc'd at
==12777==    at 0x483BE63: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==12777==    by 0x1093C0: SharedPtr<int>::SharedPtr(int*) (in /home/baseoleph/git/stepik/1_7_CPP/a.out)
==12777==    by 0x109242: main (in /home/baseoleph/git/stepik/1_7_CPP/a.out)

Upvotes: 2

Views: 112

Answers (2)

Serge Ballesta
Serge Ballesta

Reputation: 149075

The problem is in operator =. You should release the previous value if any and increment the count. And you should take case of the special case where the old and assigned pointed object are the same.

As you already have a swap, you should just use the copy and swap idiom which is known to be robust to the possible corner cases:

SharedPtr& operator=(const SharedPtr &p)
{
    SharedPtr tmp(p);
    swap(tmp)
    return *this;
}

If the SharedPtr contained nothing the dtor or the temporary will do nothing, if it contained an object, the dtor will correctly decrement it and optionaly destroy it. And if you assign the same object it will cause no harm either...

Upvotes: 3

m88
m88

Reputation: 1988

Shouldn't your assignment operator decrement c_ and possibly release p_?
And increment the new c_ value;

Upvotes: 5

Related Questions