Sign in to follow this  

check out my smart pointer class

This topic is 3287 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

I wrote a reference-counted smart pointer. Please have a look and let me know if you see any problems. The template specialization is to force only pointer types to be allowed. It could use an operator* or GetPtr function to get the raw pointer but I am not sure I'll need it. It could also use a way to set a pointer to null, possibly operator=(int anIntEqualToNull). I would like to hide the InstanceCounted class from the outside world but the only way I can think of is to make all of its methods private and make SmartPtr a friend but then if I write another SmartPtr class the new one will have to be added also. Any advice is welcome. Thanks
#pragma once
#ifndef SMARTPOINTERS_H
#define SMARTPOINTERS_H

#include <cassert>

template<typename T>
class InstanceCounted
{
	InstanceCounted();
	InstanceCounted(const InstanceCounted&);
};

template<typename T>
class InstanceCounted<T*>
{
public:
	explicit InstanceCounted(T* pT) : m_pT(pT)
	{
		assert(m_pT != null);
		m_instCount = 1;
	}

	~InstanceCounted()
	{ delete m_pT; }

	void Increment()
	{ ++m_instCount; }

	void Decrement()
	{ --m_instCount; }

	unsigned GetCount()
	{ return m_instCount; }

	T* const GetPtr()
	{ return m_pT; }
	
private:
	InstanceCounted(const InstanceCounted<T*>&);
	InstanceCounted& operator=(const InstanceCounted&);

	T* const m_pT;
	unsigned m_instCount;
};

template<typename T>
class SmartPtr
{
	SmartPtr();
	SmartPtr(const SmartPtr<T>&);
};

template<typename T>
class SmartPtr<T*>
{
public:
	explicit SmartPtr(T* pT)
	{
		m_pInstCounted = new InstanceCounted<T*>(pT);
	}

	explicit SmartPtr(const SmartPtr<T*>& rhs)
	{
		m_pInstCounted = rhs.m_pInstCounted;
		m_pInstCounted->Increment();
	}

	~SmartPtr()
	{
		m_pInstCounted->Decrement();
		if(m_pInstCounted->GetCount() == 0)
		{
			delete m_pInstCounted;
		}
	}

	SmartPtr<T*>& operator=(const SmartPtr<T*>& rhs)
	{
		if(&rhs != this)
		{
			m_pInstCounted->Decrement();
			if(m_pInstCounted->GetCount() == 0)
			{
				delete m_pInstCounted;
			}

			m_pInstCounted = rhs.m_pInstCounted;
			m_pInstCounted->Increment();
		}

		return *this;
	}

	bool operator==(const SmartPtr<T*>& rhs)
	{
		return m_pInstCounted->GetPtr() == rhs.m_pInstCounted->GetPtr();
	}

	bool operator!=(const SmartPtr<T*>& rhs)
	{
		return m_pInstCounted->GetPtr() != rhs.m_pInstCounted->GetPtr();
	}

	T* operator->()
	{
		assert(m_pInstCounted != null);
		return m_pInstCounted->GetPtr();
	}

	const T* operator->() const
	{
		assert(m_pInstCounted != null);
		return m_pInstCounted->GetPtr();
	}

	unsigned GetCount()
	{ return m_pInstCounted->GetCount(); }

private:
	InstanceCounted<T*>* m_pInstCounted;
};

#endif //SMARTPOINTERS_H

Share this post


Link to post
Share on other sites
Quote:
Original post by Rydinare
Sorry for stating the obvious, but why not use boost::shared_ptr<>?


Practice, mostly. I figured it would be a relatively small amount of code that could be written relatively quickly. boost::shared_ptr is probably better, but then I don't get the practice.

Share this post


Link to post
Share on other sites
First bug I saw: You leak memory if the allocation of the count fails.

Moral of the story: Smart pointers are hard to write. Use somebody else's -- especially now that GCC and MSVS both provide std::tr1::shared_ptr.

Share this post


Link to post
Share on other sites
Quote:
Original post by impulsionaudio
Quote:
Original post by Rydinare
Sorry for stating the obvious, but why not use boost::shared_ptr<>?


Practice, mostly. I figured it would be a relatively small amount of code that could be written relatively quickly. boost::shared_ptr is probably better, but then I don't get the practice.


Good. That's the answer I was hoping for. Usually I get silly answers like "Boost is too complicated"

Anyway, looking it over, I think the template specialization is unnecessary. I'm assuming you're anticipating someone using it like this:

SmartPtr<int*>

?

Most smart pointer classes do it like this: SmartPtr<int> and the * is implicit.

Other than that and the potential memory leak mentioned above, I think your implementation is reasonable and I don't see any obvious flaws, but I can't say for sure without giving it a run. I think the main thing would be to try some test cases and see if it works.

To be fair, also, I actually wrote my own smart pointer template class for one of my classes, because we weren't allowed to force the teacher to have to download any extra libraries (boost). I created something similar to this.

Share this post


Link to post
Share on other sites
Quote:
Original post by impulsionaudio

I would like to hide the InstanceCounted class from the outside world but the only way I can think of is to make all of its methods private and make SmartPtr a friend but then if I write another SmartPtr class the new one will have to be added also.


class SmartPtr {
struct InstanceCounted {};
};

Share this post


Link to post
Share on other sites
Additionally...

# There is no mechanism to swap pointers.
# You cannot use a custom allocator or deallocator. This is a big thing, since many games use them.
# It looks like it could have problems for SmartPtr<foo> converting to SmartPtr<const foo>

Share this post


Link to post
Share on other sites
After a bit more thought...

Consider adding operator bool()
Consider adding operator != for T*
Consider adding operator < for sorting purposes
Consider adding operator * to return a reference the object

Consider const correctness generally. Only your operator -> mostly works on a constant object, except it forces the return type to be constant. This may not be what you intended. Just because the smart pointer is constant doesn't mean the thing being pointed to should also be constant. The comparison operations (==, !=, and <) and the informational function get_count should be const since they are perfectly acceptable operations for constant pointers.

Share this post


Link to post
Share on other sites
One thing that your pointer lacks is support for natural pointer conversions e.g.


SmartPtr<Derived> d = ...;
SmartPtr<Base> b = d;

// or even:

SmartPtr<anything> a = ...
SmartPtr<const void> v = a;
// (though it's debatable as to whether you'd really want this one)


There are two ways to achieve this. One is to rearrange SmartPtr such that it's private members are:


class SmartPtr
{
std::size_t *m_instCount;
T *m_pT;
};


This means you would also have to dynamically allocate the reference count. This may or may not be a big deal for you, depending on your usage patterns.

The second way is to remove the template parameter from InstanceCounted and have it hold the count and a pointer-to-void. Then in the SmartPtr member functions you can cast to-and-fro. It feels ickier, but it is still type-safe as far as clients are concerned. You'd have to add some compile-time checks for pointer-to-derived/pointer-to-base conversions, but it's do-able.

You should also only have a single operator->, which is a const member function returning a pointer to T (non-const T). Having the const version is pointless because anyone can do this:


const SmartPtr<T> p = ...;
SmartPtr<T> p2(p);


I now have a pointer (p2) that refers to the same thing as the const SmartPtr, so I'm free to call operator-> which acts on the same underlying pointee.

Something else you could try is to add support for a custom "deleter". This actually comes in handy for passing smart-pointers across DLL boundaries, even if the DLL has been compiled with a different C runtime. I'll leave you to figure that one out, though. It's a very-nice-to-have, IMHO.

Share this post


Link to post
Share on other sites
Quote:
Original post by me22
First bug I saw: You leak memory if the allocation of the count fails.

Moral of the story: Smart pointers are hard to write. Use somebody else's -- especially now that GCC and MSVS both provide std::tr1::shared_ptr.


I am trying to use std::tr1::shared_ptr and have included <memory> but VS gives the following error:

error C3083: 'tr1': the symbol to the left of a '::' must be a type,

when making the following statement:

typedef std::tr1::shared_ptr<SomeType> SomeTypePtr;

I looked at memory and the files it includes and there is no declaration of tr1 or shared_ptr. Is the VS implementation for tr1 an add-on I need to install?

Thanks.

Share this post


Link to post
Share on other sites
Quote:
Original post by impulsionaudio
Quote:
Original post by me22
First bug I saw: You leak memory if the allocation of the count fails.

Moral of the story: Smart pointers are hard to write. Use somebody else's -- especially now that GCC and MSVS both provide std::tr1::shared_ptr.


I am trying to use std::tr1::shared_ptr and have included <memory> but VS gives the following error:

error C3083: 'tr1': the symbol to the left of a '::' must be a type,

when making the following statement:

typedef std::tr1::shared_ptr<SomeType> SomeTypePtr;

I looked at memory and the files it includes and there is no declaration of tr1 or shared_ptr. Is the VS implementation for tr1 an add-on I need to install?

Thanks.


You will need Visual Studio 2008 and the latest service pack. I don't believe there is a SP or addition for 2005 or previous.

Share this post


Link to post
Share on other sites
You may delete #pragma once since it is compiler dependent and you already have ifdef guards.

SmartPtr::GetCount should not exist since it is an implementation detail not directly related to the SmartPtr (or at least call it GetCountReferencesToInternalObject, GetCount alone is not very descriptive).

You should implement operator* which returns *m_pInstCounted and get() or GetPtr() which returns m_pInstCounted, and maybe also release() and reset() (see boost::shared_ptr).

About constness:
operator== and operator!= (and the famelic GetCOunt) should be const member functions.

Expanding frob thoughts, implementing "T* SmartPtr<T*>::operator->() const" you would break the rule "const should not expose non const", but since the assignment operator and the copy constructor already break it, there is no sense trying to preserve constness. Here how to break constness with your implementation:
void func(const SmartPtr<T*>& p)
{
//p->NonConst(); // forbidden!
SmartPtr<T*> q(p);
q->NonConst(); // allowed!
}

To preserve constness you have to implement assignment operators and copy constructors in a different way. Something like:

SmartPtr<T*>::operator=(SmartPtr<T*>& other);
SmartPtr<const T*>::operator=(const SmartPtr<T*>& other);
SmartPtr<const T*>::operator=(const SmartPtr<T*>& other);

Or

SmartPtr<T*>::operator=(SmartPtr<T*>& other);
ConstSmartPtr<T*>::operator=(const SmartPtr<T*>& other);
ConstSmartPtr<T*>::operator=(const ConstSmartPtr<T*>& other);

But that's so hard to implement, use and maintain that java and c# do not even have the c++ concept of constness.

Share this post


Link to post
Share on other sites
Quote:
Original post by Nyarlath
You may delete #pragma once since it is compiler dependent and you already have ifdef guards.


May; but it doesn't really hurt, and it might help the "dependent" compilers build the project faster. (This isn't a reason to spontaneously start using them in a new project, though. YAGNI, etc.)

Share this post


Link to post
Share on other sites
Quote:
Original post by Rydinare
Sorry for stating the obvious, but why not use boost::shared_ptr<>?
One thing to note in comparison to shared_ptr is that above implementation does not have the same thread safety guarantees that shared_ptr has.
Now, think about it as you may. In my opinion, that is even an advantage, because it's functionality that I've never used and that's associated with overhead.
However, if it happens that you do need those guarantees, you'll spend weeks trying to figure out why your program crashes.

Share this post


Link to post
Share on other sites

This topic is 3287 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this