Back to General and Gameplay Programming

What's wrong with my smart pointer?

General and Gameplay Programming Programming

Started by Evil Steve July 16, 2006 03:51 PM

12 comments, last by Deyja 17 years, 9 months ago

Evil Steve

2,021

Author

July 16, 2006 03:51 PM

Hi all, I'm writing my own smart pointer class, mainly for the experience (So please no replies of "use boost::shared_pointer" or whatever [smile]). However, MSVC 2005 doesn't seem to like it very much. It works fine, but it won't cast between objects. For example, this doesn't work:


class CBase
{
public:
   CBase() {}
   virtual ~CBase() {}
};

class CDerived : public CBase
{
public:
   CDerived() {}
   virtual ~CDerived() {}
};

void Foo(SmartPointer<CBase> ptr)
{
}

SmartPointer<CDerived> pDerived = new CDerived;
Foo(pDerived);

I get this error on the last line:

Quote: error C2664: 'Foo' : cannot convert parameter 1 from 'SmartPointer<T>' to 'SmartPointer<T>' with [T=CDerived] and [T=CBase] No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

However, changing to call to Foo() to this, works: Foo(SmartPointer<CBase>(pDerived)); So why can't it do an implicit conversion itself? Here's the entire source of my smart pointer class:


//==========================================================================
// SmartPointer.h - Smart pointer classes
//==========================================================================

#ifndef __SMARTPOINTER_H__
#define __SMARTPOINTER_H__

#include "Assert.h"

//==========================================================================
// Smart pointer base class. All classes using smart pointers must
// derive from this class
//==========================================================================
class CRefCounted
{
public:
	CRefCounted() : m_nRefCount(0) {}
	virtual ~CRefCounted() {Assert(m_nRefCount == 0);}

	// Increment the reference count for this object
	void addRef() {++m_nRefCount;}

	// Decrement the reference count for this object
	void decRef()
	{
		if(--m_nRefCount == 0)
			delete this;
		else
			Assert(m_nRefCount > 0);
	}

private:
	int	m_nRefCount;
};

//==========================================================================
// Smart pointer template class
//==========================================================================
template<class T> class SmartPointer
{
public:
	SmartPointer() : m_pObj(0) {}
	SmartPointer(const SmartPointer& rhs) : m_pObj(rhs.m_pObj)
		{if(m_pObj) m_pObj->addRef();}
	SmartPointer(T* rhs) : m_pObj(rhs)
		{if(m_pObj) m_pObj->addRef();}
	~SmartPointer() {if(m_pObj) m_pObj->decRef();}

	SmartPointer& operator=(T* rhs)
	{
		if(m_pObj) m_pObj->decRef();
		m_pObj = rhs;
		if(m_pObj) m_pObj->addRef();
		return *this;
	}

	SmartPointer& operator=(const SmartPointer& rhs)
	{
		if(m_pObj) m_pObj->decRef();
		m_pObj = rhs.m_pObj;
		if(m_pObj) m_pObj->addRef();
		return *this;
	}

	T& operator*() const {return *m_pObj;} 
	T* operator->() const {return m_pObj;}
	operator T*() const {return m_pObj;}

private:
	T* m_pObj;
};

#endif /* __SMARTPOINTER_H__ */

In the code above that does work, the code calls operator T*(), then SmartPointer(T* rhs). So why the hell can't it do that anyway on it's own? The two types are derived, damnit...

Andrew Russell

1,394

July 16, 2006 04:00 PM

You want to add something like:

friend class SmartPointer;template<class U>SmartPointer(const SmartPointer<U>& rhs) : m_pObj(rhs.m_pObj)		{if(m_pObj) m_pObj->addRef();}

I'm not quite sure why the "friend" is in my code... I can't remember if it's necessary.

AndrewRussell.net

jflanglois

1,020

July 16, 2006 04:04 PM

This is because SmartPointer< CBase > and SmartPointer< CDerived > don't derive from each other. You're going to have to write constructors that mimmick the behavior of assigning pointers to derived objects to pointers of base objects:

template< class OtherType >SmartPointer( SmartPointer< OtherType > const &sp ) : m_pObj( sp.m_pObj ) {  // adjust reference count}

Or something along those lines. You will probably want to do it for raw pointers as well.

[edit: Argh, beaten. Andrew Russell is right about friend, though. You will need it to access the member pointer and reference count of the other SmartPointer. It needs to be template< class OtherType > friend class SmartPointer;, though]

[edit2: You seem to expect that your pointed to type has addRef and decRef() members. I don't think this is how you should be doing it.]

jfl.

Evil Steve

2,021

Author

July 16, 2006 04:11 PM

Ah, that compiles now (The friend is needed because SmartPointer<T> is different from SmartPointer<U>, and they need to access the m_pObj member of each other.

I now get an interesting error elsewhere in my code:

SmartPointer<CResource> pNew = PrivateCreateResource(eType, strName);// And...SmartPointer<CResource> CResourceManager::PrivateCreateResource(CResource::Type eType, const std::string& strName){   SmartPointer<CResource> pRet;   // Code here   return pRet; // FOOM}

On the line marked "FOOM", the destructor for the smart pointer is called, which sets the reference count to 0. Then the smart pointer is assigned to pNew, which is now a bad pointer. My operator=() is never called. I tried adding more <T>'s for the return type and parameter of operator=(), still no joy.

EDIT: This replicates it too:

class CBase : public CRefCounted{public:	CBase() {}	virtual ~CBase() {}};SmartPointer<CBase> Foo(){	SmartPointer<CBase> bar;	bar = new CBase;	return bar;}SmartPointer<CBase> lol = Foo();

jflanglois

1,020

July 16, 2006 04:27 PM

I think it would help if you posted your revised SmartPointer as well as CRefCounted.

Evil Steve

2,021

Author

July 16, 2006 04:29 PM

Quote:Original post by jflanglois
I think it would help if you posted your revised SmartPointer as well as CRefCounted.

//==========================================================================//// SmartPointer.h - Smart pointer classes//==========================================================================//#ifndef __SMARTPOINTER_H__#define __SMARTPOINTER_H__#include "Assert.h"//==========================================================================//// Smart pointer base class. All classes using smart pointers must// derive from this class//==========================================================================//class CRefCounted{public:	CRefCounted() : m_nRefCount(0) {}	virtual ~CRefCounted() {Assert(m_nRefCount == 0);}	// Increment the reference count for this object	void addRef() {++m_nRefCount;}	// Decrement the reference count for this object	void decRef()	{		if(--m_nRefCount == 0)			delete this;		else			Assert(m_nRefCount > 0);	}	// Return the number of references to this object. Only for debugging	int GetRef() const {return m_nRefCount;}private:	int	m_nRefCount;};//==========================================================================//// Smart pointer template class//==========================================================================//template<class T> class SmartPointer{	friend class SmartPointer;public:	SmartPointer() : m_pObj(0) {}	template<class U> SmartPointer(const SmartPointer<U>& rhs) : m_pObj(rhs.m_pObj)		{if(m_pObj) m_pObj->addRef();}	SmartPointer(T* rhs) : m_pObj(rhs)		{if(m_pObj) m_pObj->addRef();}	~SmartPointer() {if(m_pObj) m_pObj->decRef();}	SmartPointer<T>& operator=(T* rhs)	{		if(m_pObj) m_pObj->decRef();		m_pObj = rhs;		if(m_pObj) m_pObj->addRef();		return *this;	}	SmartPointer<T>& operator=(const SmartPointer<T>& rhs)	{		if(m_pObj) m_pObj->decRef();		m_pObj = rhs.m_pObj;		if(m_pObj) m_pObj->addRef();		return *this;	}	T& operator*() const {return *m_pObj;} 	T* operator->() const {return m_pObj;}	operator T*() const {return m_pObj;}private:	T* m_pObj;};#endif /* __SMARTPOINTER_H__ */

EDIT: D'oh, it might help if I kept the original copy constructor...

Solved now, thanks [smile]

iMalc

2,466

July 17, 2006 02:07 AM

I hope you're aware that the way you've written your copy constructor, it isn't safe to do self-assignment, as the contained pointer may deleted part way through.
Also, if decRef ever throws an exception e.g. because the pointer value was corrupted, then that would cause your destructor to throw an exception, which would really cause your program to do poops. Some smart pointers remember original contained member, NULL the member, then delete what it remembered. That way if the deletion somehow causes an exception then you're program won't comletely blow chunks. At least I think that's the purpose of it. There's a lot you can learn from looking at existing smart pointer definitions.

"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms

Evil Steve

2,021

Author

July 17, 2006 03:34 AM

Quote:Original post by iMalc
I hope you're aware that the way you've written your copy constructor, it isn't safe to do self-assignment, as the contained pointer may deleted part way through.
Also, if decRef ever throws an exception e.g. because the pointer value was corrupted, then that would cause your destructor to throw an exception, which would really cause your program to do poops. Some smart pointers remember original contained member, NULL the member, then delete what it remembered. That way if the deletion somehow causes an exception then you're program won't comletely blow chunks. At least I think that's the purpose of it. There's a lot you can learn from looking at existing smart pointer definitions.

Yeah, AR pointed that out in This Does Not Exist. I'll fix that tomorrow. As for exceptions - I just avoid them like the plague.

Cheers,
Steve

deffer

755

July 17, 2006 08:50 AM

What also differs your implementation from boost's one, is that you embeed your "m_nRefCount" into the class being hold. And this forbids you from holding pointers to "const CDerived".

You could fix this by adding "const" to addRef() and decRef() and making m_nRefCount mutable.

NotAYakk

876

July 17, 2006 09:47 AM

Some more advice:

Add in the following public methods:

T* examine_ptr(); // return t;T* steal_ptr(); // zero, return t;T* copy_ptr(); // add ref, return t;T*& change_ptr(); // remove ref, zero, return ref to t;void swap(SmartPointer<T>&); // swaps!

change_ptr() is useful for functions that fill in a T*& or a T**.

copy_ptr() can be used by both other RefCount objects, and if you are passing a pointer to someone who isn't using the RefCount object.

examine_ptr() is how someone can get a T* from you without casting. I usually make my operator T* (and the like) call examine_ptr().

steal_ptr() does exactly what it says -- it steals the pointer and the reference from the object.

Lastly, swap is very useful. For one thing, it makes really easy to write operator='s:

template<typename other_type>myself& operator=(other_type const& other) {  myself tmp = other;  this->swap(tmp);  return *this;}

and now anything that you can be copy-constructed from you can also be operator='d from.

What's wrong with my smart pointer?

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

What's wrong with my smart pointer?

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines