current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 0 down vote favorite 
#include <iostream>

template <typename T>
class my_smart_ptr {
    T* raw;
    unsigned * p_count;
public:
    my_smart_ptr(T* t):
    raw(t),
    p_count (new unsigned(1))
    {

    }

    my_smart_ptr(const my_smart_ptr<T> & p):
    raw(p.raw),
    p_count(p.p_count)
    {
        ++(*p_count);
    }

    const my_smart_ptr<T> & operator=(const my_smart_ptr<T>& p)
    {
        if (p == *this)
        {
            return *this;
        }

        raw = p.raw;
        --(*p_count);
        ++(*(p.p_count));
        return *this;

    }

    ~my_smart_ptr()
    {
        --(*p_count);
        if (*(p_count) == 0)
        {
            delete raw;
        }
    }
};


class A{
public:

    A()
    {
        std::cout<< "A()" << std::endl;
    }

    ~A()
    {
        std::cout<< "~A()" << std::endl;
    }

};
int main()
{
    my_smart_ptr<A> a (new A);
    return 0;
}
share|improve this question
9  
Welcome to Code Review! It's usually helpful to provide a text introduction to your code in addition to just the title. What is the code supposed to do? What were the design goals? Does it work as you expect? See the Help Center for more details on how to ask good questions here. –  Edward May 10 at 0:48
1  
Worth reading: lokiastari.com/blog/2014/12/30/… :-) –  Loki Astari May 10 at 15:48

2 Answers 2

up vote 1 down vote accepted

I suppose that your class is trying to simulate a shared_ptr, however, I'm missing the part of overloading the dereference methods * and ->, because without them, your class seems useless (I can't do much with the pointed object).

share|improve this answer
up vote 1 down vote

You have an error in operator=. When the function returns, the p_count member of the LHS still points to the old p_count. It should be equal to the p_count of the RHS. Also you forget to release the old data when the count reaches zero.

const my_smart_ptr<T> & operator=(const my_smart_ptr<T>& p)
{
    if (p == *this)
    {
        return *this;
    }

    // Keep a copy of the old values.
    // Don't change these until the state of the
    // object has been made consistent.
    unsigned*  oldCount = p_count;
    T*         oldRaw   = raw;

    // Update the state of the object.
    // Making sure to get the local count correct.
    raw = p.raw;
    p_count = p.p_count;  // Without this the use count is incorrect.
    ++(*(p.p_count));

    // Now clean up the old state.
    --*oldCount;
    if (*oldCount == 0) {
        delete oldCount;
        delete oldRaw;
    }
    return *this;
}

It is customary for the operator= function to return a non-const reference. After all, you just modified the object.

my_smart_ptr<T> & operator=(const my_smart_ptr<T>& p) { ... }
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.