Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C++ > How can I improve this code?

Reply
Thread Tools

How can I improve this code?

 
 
Tom Smith
Guest
Posts: n/a
 
      10-14-2006
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;
 
Reply With Quote
 
 
 
 
Kai-Uwe Bux
Guest
Posts: n/a
 
      10-14-2006
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
 
Reply With Quote
 
 
 
 
AnonMail2005@gmail.com
Guest
Posts: n/a
 
      10-15-2006


On Oct 14, 6:02 pm, Tom Smith <(E-Mail Removed)> 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.

 
Reply With Quote
 
Gianni Mariani
Guest
Posts: n/a
 
      10-15-2006
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;

 
Reply With Quote
 
Tom Smith
Guest
Posts: n/a
 
      10-16-2006
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
 
Reply With Quote
 
Kai-Uwe Bux
Guest
Posts: n/a
 
      10-16-2006
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
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
Re: How include a large array? Edward A. Falk C Programming 1 04-04-2013 08:07 PM
How can i improve the look of my table easily? salmondays2000@yahoo.co.uk HTML 10 11-19-2005 10:07 PM
Sony DSC-T1 poor image quality. How can I improve? Bill Digital Photography 2 05-10-2004 08:18 AM
How can I improve streams performance? Roy Smith C++ 10 11-24-2003 11:09 PM
Can I improve my MJPG VIDEO Quality with software? lbbs Digital Photography 6 07-02-2003 06:37 PM



Advertisments