473,327 Members | 1,952 Online
Bytes | Software Development & Data Engineering Community
Post Job

Home Posts Topics Members FAQ

Join Bytes to post your question to a community of 473,327 software developers and data experts.

How can I improve this code?

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) { };
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; }
ptr<Toperator= (T* const t2)
{ Count[t]--; if (Count[t]==0 && t != null_t) delete t; Count.erase(t); t
= t2; Count[t]++; }
ptr<Toperator= (ptr<Tconst& other)
{ Count[t]--; if (Count[t]==0 && t != null_t) delete t; Count.erase(t); t
= other.t; Count[t]++; }
bool operator== (ptr<Tconst& 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*, intCount;
static T* const null_t;
};

template <class Tstd::map<T*, intptr<T>::Count;
template <class T T* const ptr<T>::null_t = 0;
Oct 14 '06 #1
5 1307
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
Oct 14 '06 #2


On Oct 14, 6:02 pm, Tom Smith <not_giving_out...@use.netwrote:
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) { };
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; }
ptr<Toperator= (T* const t2)
{ Count[t]--; if (Count[t]==0 && t != null_t) delete t; Count.erase(t); t
= t2; Count[t]++; }
ptr<Toperator= (ptr<Tconst& other)
{ Count[t]--; if (Count[t]==0 && t != null_t) delete t; Count.erase(t); t
= other.t; Count[t]++; }
bool operator== (ptr<Tconst& 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*, intCount;
static T* const null_t;

};template <class Tstd::map<T*, intptr<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 '06 #3
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 Tclass ptr
{
public:
inline ptr() : t(null_t) { };
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; }
ptr<Toperator= (T* const t2)
{ Count[t]--; if (Count[t]==0 && t != null_t) delete t;
Count.erase(t); t = t2; Count[t]++; }
ptr<Toperator= (ptr<Tconst& 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<Tconst& 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*, intCount;
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 Tstd::map<T*, intptr<T>::Count;
template <class T T* const ptr<T>::null_t = 0;
Oct 15 '06 #4
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 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.
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<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.
This was my best attempt at allowing:

class a {};

class b : public a {};

int main()
{
ptr<bb1 = 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*, 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
Cheers,
Tom
Oct 16 '06 #5
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<bb1 = 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<Sconst & other );

template < typename S >
ptr & operator= ( ptr<Sconst & 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
Oct 16 '06 #6

This thread has been closed and replies have been disabled. Please start a new discussion.

Similar topics

10
by: pembed2003 | last post by:
Hi all, I asked this question in the C group but no one seems to be interested in answering it. :-( Basically, I wrote a search and replace function so I can do: char source = "abcd?1234?x";...
3
by: Anand | last post by:
Hi Iam keen interested to improve my skills in c programming. Now Iam familiar with all the syntax and concepts. My main idea is to know how entire c compiler behaves in all circumstances(i.e...
9
by: Peng Jian | last post by:
I have a function that is called very very often. Can I improve its efficiency by declaring its local variables to be static?
23
by: philipl | last post by:
hi, I have some code here which basically look for within a string, the occurance of any 3 consectative characters which are the same. so AAA bbb etc would be reported by this function. I later...
6
by: Jéjé | last post by:
Hi, hoew can I improve the compilation process of a sharepoint website? my server is: 2 * P3 Xeom 1ghz 4go ram 2 * 36gb (mirror for OS and website) 2 * 36 Raid 0 (stripping; for temp files...
7
by: dphizler | last post by:
This is the basic concept of my code: #include <iostream.h> #include <math.h> #include <stdlib.h> int main() { double Gtotal, L, size, distance, theRandom; double Gi; int xi, xf, yi, yf,...
16
by: weidongtom | last post by:
Hi, I have just finished reading some tutorials on C, I am wondering how I could improve my skill. Is there any advice? Is reading others' codes the best way? If so, what type of codes are...
11
by: Peted | last post by:
Im using c# 2005 express edition Ive pretty much finished an winforms application and i need to significantly improve the visual appeal of the interface. Im totaly stuck on this and cant seem...
2
by: sdanda | last post by:
Hi , Do you have any idea how to improve my java class performance while selecting and inserting data into DB using JDBC Connectivity ......... This has to work for more than 8,00,000...
5
by: Gilles Ganault | last post by:
Hello I'm no PHP expert, and I'm reading "Building scalable web sites". In the tips section, the author mentions using templates to speed things up. I was wondering how the template engines...
0
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
1
isladogs
by: isladogs | last post by:
The next Access Europe meeting will be on Wednesday 6 Mar 2024 starting at 18:00 UK time (6PM UTC) and finishing at about 19:15 (7.15PM). In this month's session, we are pleased to welcome back...
0
by: Vimpel783 | last post by:
Hello! Guys, I found this code on the Internet, but I need to modify it a little. It works well, the problem is this: Data is sent from only one cell, in this case B5, but it is necessary that data...
0
by: jfyes | last post by:
As a hardware engineer, after seeing that CEIWEI recently released a new tool for Modbus RTU Over TCP/UDP filtering and monitoring, I actively went to its official website to take a look. It turned...
0
by: ArrayDB | last post by:
The error message I've encountered is; ERROR:root:Error generating model response: exception: access violation writing 0x0000000000005140, which seems to be indicative of an access violation...
1
by: PapaRatzi | last post by:
Hello, I am teaching myself MS Access forms design and Visual Basic. I've created a table to capture a list of Top 30 singles and forms to capture new entries. The final step is a form (unbound)...
1
by: CloudSolutions | last post by:
Introduction: For many beginners and individual users, requiring a credit card and email registration may pose a barrier when starting to use cloud servers. However, some cloud server providers now...
1
by: Defcon1945 | last post by:
I'm trying to learn Python using Pycharm but import shutil doesn't work
0
by: af34tf | last post by:
Hi Guys, I have a domain whose name is BytesLimited.com, and I want to sell it. Does anyone know about platforms that allow me to list my domain in auction for free. Thank you

By using Bytes.com and it's services, you agree to our Privacy Policy and Terms of Use.

To disable or enable advertisements and analytics tracking please visit the manage ads & tracking page.