How can I improve this code?

[Tom Smith]

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;

 

[Kai-Uwe Bux]

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:

{ 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

 

[AnonMail2005]

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.

 

[Gianni Mariani]

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.

 

[Tom Smith]

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:

{ 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?

Huh? Why would a pointer be a function object?

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

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

 

[Kai-Uwe Bux]

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