Is this a good reference-counting smart pointer class?

Discussion in 'C++' started by Protoman, Mar 28, 2009.

  1. Protoman

    Protoman Guest

    Is this a good reference-counting smart pointer class?

    // begin code
    -------------------------------------------------------------------------------------------------------------------
    class SmrtPtrDB
    {
    public:
    SmrtPtrDB(int status=1):num(status){}
    SmrtPtrDB(const SmrtPtrDB& rhs):num(rhs.num){}
    ~SmrtPtrDB(){}
    void add(){num++;}
    void sub(){num--;}
    int status(){return num;}
    private:
    int num;
    };

    class NullPtr{};

    template<class T>
    class SmrtPtr
    {
    public:
    explicit SmrtPtr<T>(T* obj=0):ptr(obj),DataBase(new SmrtPtrDB){}
    SmrtPtr<T>(const SmrtPtr<T>& rhs):ptr(rhs.ptr),DataBase(new
    SmrtPtrDB(rhs.DataBase->status())) {DataBase->add();}
    ~SmrtPtr<T>()
    {
    DataBase->sub();
    if(DataBase->status()==0)
    {delete ptr; delete DataBase;}
    else {delete DataBase;}
    }
    void operator=(T* val)
    {
    SmrtPtr<T> temp(val);
    swap(temp);
    }
    SmrtPtr<T>& operator=(const SmrtPtr<T>& rhs)
    {
    SmrtPtr<T> temp(rhs);
    swap(temp);
    return *this;
    }
    bool operator==(const SmrtPtr<T>& rhs)const {if(ptr==rhs.ptr)return
    true;else return false;}
    bool operator!=(const SmrtPtr<T>& rhs)const {if(ptr!=rhs.ptr)return
    true;else return false;}
    bool operator<=(const SmrtPtr<T>& rhs)const {if(ptr<=rhs.ptr)return
    true;else return false;}
    bool operator>=(const SmrtPtr<T>& rhs)const {if(ptr>=rhs.ptr)return
    true;else return false;}
    int status(){return DataBase->status();}
    T& operator*()const {if(ptr==0)throw NullPtr();else return *ptr;}
    T* operator->()const {if(ptr==0)throw NullPtr();else return ptr;}
    operator T*()const {if(ptr==0)throw NullPtr();else return ptr;}
    private:
    void swap(SmrtPtr<T>& rhs)
    {
    std::swap(DataBase,rhs.DataBase);
    std::swap(ptr,rhs.ptr);
    }
    mutable SmrtPtrDB* DataBase;
    T* ptr;
    };
    //end of code
    ---------------------------------------------------------------------------------------------------------------------------------

    How do I improve this class? Is there anything "wrong" with it?
    Protoman, Mar 28, 2009
    #1
    1. Advertising

  2. Protoman wrote:
    > explicit SmrtPtr<T>(T* obj=0):ptr(obj),DataBase(new SmrtPtrDB){}
    > SmrtPtr<T>(const SmrtPtr<T>& rhs):ptr(rhs.ptr),DataBase(new
    > SmrtPtrDB(rhs.DataBase->status())) {DataBase->add();}


    No, this definitely doesn't work.

    You are *not* sharing the 'DataBase' instance between the 'SmrtPtr'
    copies pointing to the same object. In other words, each instance of
    'SmrtPtr' has its own instance of 'DataBase'. Basically there's no
    difference between what you have done there and having a 'SmrtPtrDB'
    instance directly as a member variable (rather than it being allocated
    dynamically).

    You are copying the reference count from 'rhs' to the newly-created
    instance of 'DataBase' in the copy constructor. How do you think it's
    going to notice if that other 'SmrtPtr' is destroyed? It retains no
    connection to it whatsoever, so there's no way it can notice that it has
    disappeared.

    Moreover, that other 'SmrtPtr' instance is not going to notice that a
    new 'SmrtPtr' instance was created to point to the same object. The
    reference count of the former will be unmodified. Thus when the former
    is destroyed, it will delete the object, and not the new 'SmrtPtr'
    object will point to deleted memory.
    Juha Nieminen, Mar 28, 2009
    #2
    1. Advertising

  3. Protoman

    douglas Guest

    On Mar 28, 1:12 am, Juha Nieminen <> wrote:
    > Protoman wrote:
    > > explicit SmrtPtr<T>(T* obj=0):ptr(obj),DataBase(new SmrtPtrDB){}
    > > SmrtPtr<T>(const SmrtPtr<T>& rhs):ptr(rhs.ptr),DataBase(new
    > > SmrtPtrDB(rhs.DataBase->status())) {DataBase->add();}

    >
    >   No, this definitely doesn't work.
    >
    >   You are *not* sharing the 'DataBase' instance between the 'SmrtPtr'
    > copies pointing to the same object. In other words, each instance of
    > 'SmrtPtr' has its own instance of 'DataBase'. Basically there's no
    > difference between what you have done there and having a 'SmrtPtrDB'
    > instance directly as a member variable (rather than it being allocated
    > dynamically).
    >
    >   You are copying the reference count from 'rhs' to the newly-created
    > instance of 'DataBase' in the copy constructor. How do you think it's
    > going to notice if that other 'SmrtPtr' is destroyed? It retains no
    > connection to it whatsoever, so there's no way it can notice that it has
    > disappeared.
    >
    >   Moreover, that other 'SmrtPtr' instance is not going to notice that a
    > new 'SmrtPtr' instance was created to point to the same object. The
    > reference count of the former will be unmodified. Thus when the former
    > is destroyed, it will delete the object, and not the new 'SmrtPtr'
    > object will point to deleted memory.


    So, how do I fix it?
    douglas, Mar 28, 2009
    #3
  4. douglas wrote:
    > So, how do I fix it?


    You either need an intrusive reference count or a manager object that is
    shared by all instances of the smart pointer that point to the same object.

    I prefer the first, if I have the choice.


    Marcel
    Marcel Müller, Mar 28, 2009
    #4
  5. Protoman

    Kram Guest

    On Mar 28, 3:32 am, Protoman <> wrote:
    > Is this a good reference-counting smart pointer class?
    >
    > // begin code
    > -------------------------------------------------------------------------------------------------------------------
    > class SmrtPtrDB
    > {
    > public:
    > SmrtPtrDB(int status=1):num(status){}
    > SmrtPtrDB(const SmrtPtrDB& rhs):num(rhs.num){}
    > ~SmrtPtrDB(){}
    > void add(){num++;}
    > void sub(){num--;}
    > int status(){return num;}
    > private:
    > int num;
    >
    > };
    >
    > class NullPtr{};
    >
    > template<class T>
    > class SmrtPtr
    > {
    > public:
    > explicit SmrtPtr<T>(T* obj=0):ptr(obj),DataBase(new SmrtPtrDB){}
    > SmrtPtr<T>(const SmrtPtr<T>& rhs):ptr(rhs.ptr),DataBase(new
    > SmrtPtrDB(rhs.DataBase->status())) {DataBase->add();}
    > ~SmrtPtr<T>()
    > {
    > DataBase->sub();
    > if(DataBase->status()==0)
    > {delete ptr; delete DataBase;}
    > else {delete DataBase;}}
    >
    > void operator=(T* val)
    > {
    > SmrtPtr<T> temp(val);
    > swap(temp);}
    >
    > SmrtPtr<T>& operator=(const SmrtPtr<T>& rhs)
    > {
    > SmrtPtr<T> temp(rhs);
    > swap(temp);
    > return *this;}
    >
    > bool operator==(const SmrtPtr<T>& rhs)const {if(ptr==rhs.ptr)return
    > true;else return false;}
    > bool operator!=(const SmrtPtr<T>& rhs)const {if(ptr!=rhs.ptr)return
    > true;else return false;}
    > bool operator<=(const SmrtPtr<T>& rhs)const {if(ptr<=rhs.ptr)return
    > true;else return false;}
    > bool operator>=(const SmrtPtr<T>& rhs)const {if(ptr>=rhs.ptr)return
    > true;else return false;}
    > int status(){return DataBase->status();}
    > T& operator*()const {if(ptr==0)throw NullPtr();else return *ptr;}
    > T* operator->()const {if(ptr==0)throw NullPtr();else return ptr;}
    > operator T*()const {if(ptr==0)throw NullPtr();else return ptr;}
    > private:
    > void swap(SmrtPtr<T>& rhs)
    > {
    > std::swap(DataBase,rhs.DataBase);
    > std::swap(ptr,rhs.ptr);}
    >
    > mutable SmrtPtrDB* DataBase;
    > T* ptr;};
    >
    > //end of code
    > ---------------------------------------------------------------------------------------------------------------------------------
    >
    > How do I improve this class? Is there anything "wrong" with it?


    It seems like you're design is to create a database that keep track of
    addresses being referenced by pointers in a class. That way you can
    know how many references there are to a memory address. The problem is
    you create a new copy of the database for each pointer, you need to
    look into the Singleton method for the database, and use the factory
    method to generate pointers. This factory can either contain or
    reference the Database and that way you ensure only one shared
    database.
    Kram, Mar 28, 2009
    #5
  6. Protoman

    douglas Guest

    On Mar 28, 2:11 pm, Kram <> wrote:
    > On Mar 28, 3:32 am, Protoman <> wrote:
    >
    >
    >
    >
    >
    > > Is this a good reference-counting smart pointer class?

    >
    > > // begin code
    > > ---------------------------------------------------------------------------­----------------------------------------
    > > class SmrtPtrDB
    > > {
    > > public:
    > > SmrtPtrDB(int status=1):num(status){}
    > > SmrtPtrDB(const SmrtPtrDB& rhs):num(rhs.num){}
    > > ~SmrtPtrDB(){}
    > > void add(){num++;}
    > > void sub(){num--;}
    > > int status(){return num;}
    > > private:
    > > int num;

    >
    > > };

    >
    > > class NullPtr{};

    >
    > > template<class T>
    > > class SmrtPtr
    > > {
    > > public:
    > > explicit SmrtPtr<T>(T* obj=0):ptr(obj),DataBase(new SmrtPtrDB){}
    > > SmrtPtr<T>(const SmrtPtr<T>& rhs):ptr(rhs.ptr),DataBase(new
    > > SmrtPtrDB(rhs.DataBase->status())) {DataBase->add();}
    > > ~SmrtPtr<T>()
    > > {
    > > DataBase->sub();
    > > if(DataBase->status()==0)
    > > {delete ptr; delete DataBase;}
    > > else {delete DataBase;}}

    >
    > > void operator=(T* val)
    > > {
    > > SmrtPtr<T> temp(val);
    > > swap(temp);}

    >
    > > SmrtPtr<T>& operator=(const SmrtPtr<T>& rhs)
    > > {
    > > SmrtPtr<T> temp(rhs);
    > > swap(temp);
    > > return *this;}

    >
    > > bool operator==(const SmrtPtr<T>& rhs)const {if(ptr==rhs.ptr)return
    > > true;else return false;}
    > > bool operator!=(const SmrtPtr<T>& rhs)const {if(ptr!=rhs.ptr)return
    > > true;else return false;}
    > > bool operator<=(const SmrtPtr<T>& rhs)const {if(ptr<=rhs.ptr)return
    > > true;else return false;}
    > > bool operator>=(const SmrtPtr<T>& rhs)const {if(ptr>=rhs.ptr)return
    > > true;else return false;}
    > > int status(){return DataBase->status();}
    > > T& operator*()const {if(ptr==0)throw NullPtr();else return *ptr;}
    > > T* operator->()const {if(ptr==0)throw NullPtr();else return ptr;}
    > > operator T*()const {if(ptr==0)throw NullPtr();else return ptr;}
    > > private:
    > > void swap(SmrtPtr<T>& rhs)
    > > {
    > > std::swap(DataBase,rhs.DataBase);
    > > std::swap(ptr,rhs.ptr);}

    >
    > > mutable SmrtPtrDB* DataBase;
    > > T* ptr;};

    >
    > > //end of code
    > > ---------------------------------------------------------------------------­------------------------------------------------------

    >
    > > How do I improve this class? Is there anything "wrong" with it?

    >
    > It seems like you're design is to create a database that keep track of
    > addresses being referenced by pointers in a class. That way you can
    > know how many references there are to a memory address. The problem is
    > you create a new copy of the database for each pointer, you need to
    > look into the Singleton method for the database, and use the factory
    > method to generate pointers. This factory can either contain or
    > reference the Database and that way you ensure only one shared
    > database.- Hide quoted text -
    >
    > - Show quoted text -


    I know the Singleton pattern, but what's the factory pattern? Can you
    give me an example?
    douglas, Mar 28, 2009
    #6
  7. douglas wrote:
    > So, how do I fix it?


    Share the reference counter with all the copies, rather than
    replicating it.
    Juha Nieminen, Mar 29, 2009
    #7
  8. Protoman

    douglas Guest

    On Mar 29, 12:46 am, Juha Nieminen <> wrote:
    > douglas wrote:
    > > So, how do I fix it?

    >
    >   Share the reference counter with all the copies, rather than
    > replicating it.


    Or could I modify the classes to have a mutable ref count variable,
    and have the SmrtPtr class add and subtract from that, and free the
    ptr when that var hits 0. Would that be intrusive ref counting?
    douglas, Mar 29, 2009
    #8
  9. Protoman

    James Kanze Guest

    On Mar 29, 11:48 pm, douglas <> wrote:
    > On Mar 29, 12:46 am, Juha Nieminen <> wrote:


    > > douglas wrote:
    > > > So, how do I fix it?


    > > Share the reference counter with all the copies, rather than
    > > replicating it.


    > Or could I modify the classes to have a mutable ref count
    > variable, and have the SmrtPtr class add and subtract from
    > that, and free the ptr when that var hits 0. Would that be
    > intrusive ref counting?


    That's what is meant by intrusive reference counting, yes.

    The version having the separate counter object (from Barton and
    Nackman, for example) has the advantage that it works with all
    types of objects, even ones in the standard or some third party
    library. On the other hand, it is extremely fragile---it's far
    too easy to end up with two counter objects. For this reason, I
    would avoid it at all cost in critical code. The invasive
    version (from Scott Meyers, for example) typically requires that
    the pointed to object ultimately derive from a base class
    containing the counter. This means that you can't use it on
    existing classes directly (but it's trivial to wrap an existing
    class so that you can use it); it also has implications when
    multiple inheritance is involved. On the other hand, it's
    fairly robust. (As robust as reference counting can be.)

    --
    James Kanze (GABI Software) email:
    Conseils en informatique orientée objet/
    Beratung in objektorientierter Datenverarbeitung
    9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34
    James Kanze, Mar 30, 2009
    #9
  10. Christian Kandeler wrote:
    >> operator T*()const {if(ptr==0)throw NullPtr();else return ptr;}

    >
    > In addition to what others have said, I wonder why your class throws an
    > exception on implicit conversion of a null pointer. Why should this be
    > illegal?


    I bet that what he wanted was overload operator*.
    Juha Nieminen, Mar 30, 2009
    #10
  11. Protoman

    douglas Guest

    On Mar 30, 7:06 am, Juha Nieminen <> wrote:
    > Christian Kandeler wrote:
    > >> operator T*()const {if(ptr==0)throw NullPtr();else return ptr;}

    >
    > > In addition to what others have said, I wonder why your class throws an
    > > exception on implicit conversion of a null pointer. Why should this be
    > > illegal?

    >
    >   I bet that what he wanted was overload operator*.


    I'm relatively new to this as you've guessed; I've only been coding
    for two years, for fun. I'm also 17 years old and in community college.
    douglas, Mar 30, 2009
    #11
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Neal Coombes
    Replies:
    5
    Views:
    1,154
  2. ma740988
    Replies:
    0
    Views:
    308
    ma740988
    Dec 12, 2005
  3. mathieu
    Replies:
    8
    Views:
    509
    Juha Nieminen
    Aug 31, 2008
  4. CplusplusNewbie
    Replies:
    17
    Views:
    1,790
  5. A
    Replies:
    7
    Views:
    632
Loading...

Share This Page