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 Tclass ptr
{
public:
inline ptr() : t(null_t) { };
The semicolon is poor form.
inline ptr(ptr<Tconst& 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<Tinside 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<Toperator= (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<intisp ( ip );
isp = ip;
This is bad. I would probably not allow assignment from raw pointers at all.
ptr<Toperator= (ptr<Tconst& 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<Tconst& other)
Not const correct:
bool operator== (ptr<Tconst& 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*, intCount;
static T* const null_t;
};
template <class Tstd::map<T*, intptr<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