How can I improve this code?

Discussion in 'C++' started by Tom Smith, Oct 14, 2006.

  1. Tom Smith

    Tom Smith Guest

    I'm writing my own smart pointer class (as an exercise, not for real life use);
    I've got it to a point where I think it's usable: but I'm not quite sure. Any
    comments on the class gratefully received.

    Cheers - Tom


    /** code starts here **/
    /** **/
    /** obviously ! **/

    #include <map>


    template <class T> class ptr
    {
    public:
    inline ptr() : t(null_t) { };
    inline ptr(ptr<T> const& other) : t (other.t)
    { Count[t]++; }
    inline ptr(T* const t2) : t (t2)
    { Count[t]++; }
    inline ~ptr()
    { Count[t]--; if (Count[t]==0 && t != null_t) delete t; }
    ptr<T> operator= (T* const t2)
    { Count[t]--; if (Count[t]==0 && t != null_t) delete t; Count.erase(t); t
    = t2; Count[t]++; }
    ptr<T> operator= (ptr<T> const& other)
    { Count[t]--; if (Count[t]==0 && t != null_t) delete t; Count.erase(t); t
    = other.t; Count[t]++; }
    bool operator== (ptr<T> const& other)
    { return (t == other.t); }
    bool operator== (T* const t2)
    { return (t == t2); }
    T* operator-> () { return t; }
    operator T* () { return t; }
    T* operator() (){ return t; }
    private:
    T* t;
    static std::map<T*, int> Count;
    static T* const null_t;
    };

    template <class T> std::map<T*, int> ptr<T>::Count;
    template <class T> T* const ptr<T>::null_t = 0;
    Tom Smith, Oct 14, 2006
    #1
    1. Advertising

  2. Tom Smith

    Kai-Uwe Bux Guest

    Tom Smith wrote:

    > I'm writing my own smart pointer class (as an exercise, not for real life
    > use); I've got it to a point where I think it's usable: but I'm not quite
    > sure. Any comments on the class gratefully received.
    >
    > Cheers - Tom
    >
    >
    > /** code starts here **/
    > /** **/
    > /** obviously ! **/
    >
    > #include <map>
    >
    >
    > template <class T> class ptr
    > {
    > public:
    > inline ptr() : t(null_t) { };


    The semicolon is poor form.

    > inline ptr(ptr<T> const& other) : t (other.t)
    > { Count[t]++; }
    > inline ptr(T* const t2) : t (t2)
    > { Count[t]++; }
    > inline ~ptr()
    > { Count[t]--; if (Count[t]==0 && t != null_t) delete t; }


    The inline keywords are redundant since you provide bodies in place. Also,
    you do not need ptr<T> inside the class definition:

    ptr() : t(null_t) { }
    ptr(ptr const& other) : t (other.t)
    { Count[t]++; }
    ptr(T* const t2) : t (t2)
    { Count[t]++; }
    ~ptr()
    { Count[t]--; if (Count[t]==0 && t != null_t) delete t; }


    > ptr<T> operator= (T* const t2)


    ptr operator= (T* const t2)

    > { Count[t]--; if (Count[t]==0 && t != null_t) delete t;
    > { Count.erase(t); t
    > = t2; Count[t]++; }


    It is not quite clear what the semantics of an assignment of this form
    should be. For instance:

    int* ip = new int (4);
    ptr<int> isp ( ip );
    isp = ip;

    This is bad. I would probably not allow assignment from raw pointers at all.


    > ptr<T> operator= (ptr<T> const& other)
    > { Count[t]--; if (Count[t]==0 && t != null_t) delete t;
    > { Count.erase(t); t
    > = other.t; Count[t]++; }


    This assignment operator is bogus. It does not handle self-assignment
    correctly. The easiest way to get self-assignment right for a reference
    counted smart pointer is to use the copy-swap idiom:

    void swap ( ptr & other ) {
    std::swap( Count[t], Count[other.t] );
    std::swap( this->t, other.t );
    }

    ptr operator= (ptr const & other) {
    ptr dummy ( other );
    this->swap( dummy );
    return ( *this );
    }

    This way, you only have to think about correctness of copy-construction and
    destruction.

    It you really want assignment from raw pointers, you could do:

    ptr operator= ( T* other) {
    ptr dummy ( other );
    this->swap( dummy );
    return ( *this );
    }

    However, BadThings(tm) will still happen.


    > bool operator== (ptr<T> const& other)


    Not const correct:

    bool operator== (ptr<T> const& other) const


    > { return (t == other.t); }
    > bool operator== (T* const t2)


    Not const correct:

    bool operator== (T* const t2) const
    > { return (t == t2); }


    You should also provide operator< (using std::less<T*>) so that you can use
    the smart pointer in sets and the like.

    bool operator< ( ptr const & rhs ) const {
    return ( std::less<T*>()( this->t, rhs.t ) );
    }


    > T* operator-> () { return t; }


    Missing const version:

    T const * operator-> () const { return t; }

    Also missing:

    T & operator* () { return *t; }
    T const & operator* () const { return *t; }

    > operator T* () { return t; }


    This conversion can lead to surprises. I would ditch it.



    > T* operator() (){ return t; }


    Huh? Why would a pointer be a function object?


    > private:
    > T* t;
    > static std::map<T*, int> Count;
    > static T* const null_t;
    > };
    >
    > template <class T> std::map<T*, int> ptr<T>::Count;
    > template <class T> T* const ptr<T>::null_t = 0;



    The use of a static map is highly inefficient. Consider:

    private:
    T* t;
    unsigned int* c;

    and something like:

    ptr()
    : t(null_t)
    , c ( new unsigned int (1) )
    { }

    ptr(ptr const& other)
    : t (other.t)
    , c ( other.c )
    {
    ++ (*c);
    }

    ptr(T* other)
    : t (other)
    , c ( new unsigned int (1) )
    { }

    ~ptr() {
    -- (*c);
    if ( *c == 0 ) {
    delete t;
    delete c;
    }
    }

    Warning: that is just written of the top of my head without testing or even
    running it by a compiler.


    Best

    Kai-Uwe Bux
    Kai-Uwe Bux, Oct 15, 2006
    #2
    1. Advertising

  3. Tom Smith

    Guest

    On Oct 14, 6:02 pm, Tom Smith <> wrote:
    > I'm writing my own smart pointer class (as an exercise, not for real life use);
    > I've got it to a point where I think it's usable: but I'm not quite sure. Any
    > comments on the class gratefully received.
    >
    > Cheers - Tom
    >
    > /** code starts here **/
    > /** **/
    > /** obviously ! **/
    >
    > #include <map>
    >
    > template <class T> class ptr
    > {
    > public:
    > inline ptr() : t(null_t) { };
    > inline ptr(ptr<T> const& other) : t (other.t)
    > { Count[t]++; }
    > inline ptr(T* const t2) : t (t2)
    > { Count[t]++; }
    > inline ~ptr()
    > { Count[t]--; if (Count[t]==0 && t != null_t) delete t; }
    > ptr<T> operator= (T* const t2)
    > { Count[t]--; if (Count[t]==0 && t != null_t) delete t; Count.erase(t); t
    > = t2; Count[t]++; }
    > ptr<T> operator= (ptr<T> const& other)
    > { Count[t]--; if (Count[t]==0 && t != null_t) delete t; Count.erase(t); t
    > = other.t; Count[t]++; }
    > bool operator== (ptr<T> const& other)
    > { return (t == other.t); }
    > bool operator== (T* const t2)
    > { return (t == t2); }
    > T* operator-> () { return t; }
    > operator T* () { return t; }
    > T* operator() (){ return t; }
    > private:
    > T* t;
    > static std::map<T*, int> Count;
    > static T* const null_t;
    >
    > };template <class T> std::map<T*, int> ptr<T>::Count;
    > template <class T> T* const ptr<T>::null_t = 0


    Also, forget about that null_t stuff. Initialize the pointer to zero
    for the default
    constructor and just delete it if the reference count goes to zero.
    There is
    no danger in deleting a pointer whose value is zero.
    , Oct 15, 2006
    #3
  4. Tom Smith wrote:
    > I'm writing my own smart pointer class (as an exercise, not for real
    > life use); I've got it to a point where I think it's usable: but I'm not
    > quite sure. Any comments on the class gratefully received.


    You need to be able to do a number of other things for it to work like a
    pointer.

    e.g.

    struct A{}; struct B : A {};

    A * x;
    B * y;

    x=y; // implicit down cast

    ptr<A> x;
    ptr<B> y;

    x=y; // need to apply implicit cast.

    This can be done by templatizing the assignment and copy constructors.

    >
    > Cheers - Tom
    >
    >
    > /** code starts here **/
    > /** **/
    > /** obviously ! **/
    >
    > #include <map>
    >
    >
    > template <class T> class ptr
    > {
    > public:
    > inline ptr() : t(null_t) { };
    > inline ptr(ptr<T> const& other) : t (other.t)
    > { Count[t]++; }
    > inline ptr(T* const t2) : t (t2)
    > { Count[t]++; }
    > inline ~ptr()
    > { Count[t]--; if (Count[t]==0 && t != null_t) delete t; }
    > ptr<T> operator= (T* const t2)
    > { Count[t]--; if (Count[t]==0 && t != null_t) delete t;
    > Count.erase(t); t = t2; Count[t]++; }
    > ptr<T> operator= (ptr<T> const& other)
    > { Count[t]--; if (Count[t]==0 && t != null_t) delete t;
    > Count.erase(t); t = other.t; Count[t]++; }


    Assignment is bad. Does not manage self-assignment or circular
    references. The delete operation must be the last thing you do before
    returning.

    > bool operator== (ptr<T> const& other)
    > { return (t == other.t); }
    > bool operator== (T* const t2)
    > { return (t == t2); }
    > T* operator-> () { return t; }
    > operator T* () { return t; }
    > T* operator() (){ return t; }
    > private:
    > T* t;
    > static std::map<T*, int> Count;


    Your count methodology also does not deal with related types.

    The Count map can be a considerable performance hit. I don't know if it
    could ever work correctly. e.g.

    struct A {};

    struct B : virtual A {};

    struct C : virtual A {};

    struct D : C, B {};

    ptr<A> a;
    ptr<B> b;
    ptr<C> c;
    ptr<D> d;

    b = d;
    c = d;

    d = 0;
    b = 0; // oops proably gets deleted here.

    So, the way that boost::shared_ptr works is that it passes around a
    pointer to an object that contains a pointer to the client object and a
    reference count pus a few methods to deal with it. It is alot less
    expensve than a map but still pretty expensive.

    I prefer to use intrusive reference counting (the count is part of the
    object) - alot less fuss and very efficient.



    > static T* const null_t;
    > };
    >
    > template <class T> std::map<T*, int> ptr<T>::Count;
    > template <class T> T* const ptr<T>::null_t = 0;
    Gianni Mariani, Oct 15, 2006
    #4
  5. Tom Smith

    Tom Smith Guest

    Kai-Uwe Bux wrote:
    > Tom Smith wrote:
    >
    >> I'm writing my own smart pointer class (as an exercise, not for real life
    >> use); I've got it to a point where I think it's usable: but I'm not quite
    >> sure. Any comments on the class gratefully received.
    >>
    >> Cheers - Tom
    >>
    >>
    >> /** code starts here **/
    >> /** **/
    >> /** obviously ! **/
    >>
    >> #include <map>
    >>
    >>
    >> template <class T> class ptr
    >> {
    >> public:
    >> inline ptr() : t(null_t) { };

    >
    > The semicolon is poor form.
    >
    >> inline ptr(ptr<T> const& other) : t (other.t)
    >> { Count[t]++; }
    >> inline ptr(T* const t2) : t (t2)
    >> { Count[t]++; }
    >> inline ~ptr()
    >> { Count[t]--; if (Count[t]==0 && t != null_t) delete t; }

    >
    > The inline keywords are redundant since you provide bodies in place. Also,
    > you do not need ptr<T> inside the class definition:
    >
    > ptr() : t(null_t) { }
    > ptr(ptr const& other) : t (other.t)
    > { Count[t]++; }
    > ptr(T* const t2) : t (t2)
    > { Count[t]++; }
    > ~ptr()
    > { Count[t]--; if (Count[t]==0 && t != null_t) delete t; }
    >
    >
    >> ptr<T> operator= (T* const t2)

    >
    > ptr operator= (T* const t2)
    >
    >> { Count[t]--; if (Count[t]==0 && t != null_t) delete t;
    >> { Count.erase(t); t
    >> = t2; Count[t]++; }

    >
    > It is not quite clear what the semantics of an assignment of this form
    > should be. For instance:
    >
    > int* ip = new int (4);
    > ptr<int> isp ( ip );
    > isp = ip;
    >
    > This is bad. I would probably not allow assignment from raw pointers at all.
    >
    >
    >> ptr<T> operator= (ptr<T> const& other)
    >> { Count[t]--; if (Count[t]==0 && t != null_t) delete t;
    >> { Count.erase(t); t
    >> = other.t; Count[t]++; }

    >
    > This assignment operator is bogus.


    Yes: I discovered this almost immediately after posting.

    It does not handle self-assignment
    > correctly. The easiest way to get self-assignment right for a reference
    > counted smart pointer is to use the copy-swap idiom:
    >
    > void swap ( ptr & other ) {
    > std::swap( Count[t], Count[other.t] );
    > std::swap( this->t, other.t );
    > }
    >
    > ptr operator= (ptr const & other) {
    > ptr dummy ( other );
    > this->swap( dummy );
    > return ( *this );
    > }
    >
    > This way, you only have to think about correctness of copy-construction and
    > destruction.


    Corrected.

    >
    > It you really want assignment from raw pointers, you could do:
    >
    > ptr operator= ( T* other) {
    > ptr dummy ( other );
    > this->swap( dummy );
    > return ( *this );
    > }
    >
    > However, BadThings(tm) will still happen.
    >
    >
    >> bool operator== (ptr<T> const& other)

    >
    > Not const correct:
    >
    > bool operator== (ptr<T> const& other) const
    >
    >
    >> { return (t == other.t); }
    >> bool operator== (T* const t2)

    >
    > Not const correct:
    >
    > bool operator== (T* const t2) const
    >> { return (t == t2); }

    >
    > You should also provide operator< (using std::less<T*>) so that you can use
    > the smart pointer in sets and the like.
    >
    > bool operator< ( ptr const & rhs ) const {
    > return ( std::less<T*>()( this->t, rhs.t ) );
    > }
    >
    >
    >> T* operator-> () { return t; }

    >
    > Missing const version:
    >
    > T const * operator-> () const { return t; }
    >
    > Also missing:
    >
    > T & operator* () { return *t; }
    > T const & operator* () const { return *t; }
    >
    >> operator T* () { return t; }

    >
    > This conversion can lead to surprises. I would ditch it.
    >


    This was my best attempt at allowing:

    class a {};

    class b : public a {};

    int main()
    {
    ptr<b> b1 = new(b);
    ptr<a> = ptr<b>;
    }

    How should I do this instead?

    >
    >
    >> T* operator() (){ return t; }

    >
    > Huh? Why would a pointer be a function object?


    I have no idea whatsoever what this was doing in the code.

    >
    >
    >> private:
    >> T* t;
    >> static std::map<T*, int> Count;
    >> static T* const null_t;
    >> };
    >>
    >> template <class T> std::map<T*, int> ptr<T>::Count;
    >> template <class T> T* const ptr<T>::null_t = 0;

    >
    >
    > The use of a static map is highly inefficient. Consider:
    >
    > private:
    > T* t;
    > unsigned int* c;
    >
    > and something like:
    >
    > ptr()
    > : t(null_t)
    > , c ( new unsigned int (1) )
    > { }
    >
    > ptr(ptr const& other)
    > : t (other.t)
    > , c ( other.c )
    > {
    > ++ (*c);
    > }
    >
    > ptr(T* other)
    > : t (other)
    > , c ( new unsigned int (1) )
    > { }
    >
    > ~ptr() {
    > -- (*c);
    > if ( *c == 0 ) {
    > delete t;
    > delete c;
    > }
    > }
    >
    > Warning: that is just written of the top of my head without testing or even
    > running it by a compiler.
    >
    >
    > Best
    >
    > Kai-Uwe Bux


    Cheers,


    Tom
    Tom Smith, Oct 16, 2006
    #5
  6. Tom Smith

    Kai-Uwe Bux Guest

    Tom Smith wrote:

    > Kai-Uwe Bux wrote:
    >> Tom Smith wrote:

    [snip]
    >>> operator T* () { return t; }

    >>
    >> This conversion can lead to surprises. I would ditch it.
    >>

    >
    > This was my best attempt at allowing:
    >
    > class a {};
    >
    > class b : public a {};
    >
    > int main()
    > {
    > ptr<b> b1 = new(b);
    > ptr<a> = ptr<b>;
    > }
    >
    > How should I do this instead?

    [snip]

    What about a templated copy constructor and assignment operator:

    template < typename S >
    ptr ( ptr<S> const & other );

    template < typename S >
    ptr & operator= ( ptr<S> const & other );

    Within the bodies, you could put compile time asserts that T is polymorphic
    and that S is derived from T.


    Best

    Kai-Uwe Bux
    Kai-Uwe Bux, Oct 16, 2006
    #6
    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. Gustavo Campanelli

    Newbie question: Any way to improve this code?

    Gustavo Campanelli, Dec 5, 2003, in forum: Python
    Replies:
    10
    Views:
    439
    Gustavo Campanelli
    Dec 7, 2003
  2. py
    Replies:
    17
    Views:
    451
    Steven D'Aprano
    Jan 14, 2006
  3. Petr Jakes
    Replies:
    41
    Views:
    880
    Petr Jakes
    Jan 28, 2006
  4. Skybuck Flying
    Replies:
    65
    Views:
    1,259
    Keith Thompson
    Sep 24, 2005
  5. advancedk

    Can I improve my code?

    advancedk, Jul 27, 2008, in forum: C Programming
    Replies:
    0
    Views:
    283
    advancedk
    Jul 27, 2008
Loading...

Share This Page