irr_ptr

[SSG]

There have been some suggestions that IReferenceCounted should be removed from the API and boost::shared_ptr be used instead. After all, anyone can forget to call drop() just like anyone can forget to use the delete operator.

The following smart pointer is designed to wrap any IReferenceCounted instance, so it can be used with Irrlicht as is. It's not as fancy as boost::shared_ptr, but it's good enough for the job.

Code: Select all

#ifndef IRR_PTR_H
#define IRR_PTR_H

namespace irr
{
	template <class T>
	class irr_ptr
	{
	private:
		T* ptr;

	public:
		irr_ptr() : ptr(0) {}

		irr_ptr(T* ptr) : ptr(ptr) { }

		irr_ptr(const irr_ptr& r) : ptr(r.ptr) { if (ptr) ptr->grab(); }

		template <class Y> irr_ptr(const irr_ptr<Y>& r) : ptr(r.ptr) { if (ptr) ptr->grab(); }

		~irr_ptr() { if (ptr) ptr->drop(); }

		T* get() { return ptr; }
		T* operator->() { return ptr; }
		T& operator*()  { return *ptr; }
		
		const T* get() const { return ptr; }
		const T* operator->() const { return ptr; }
		const T& operator*() const { return *ptr; }
		
		bool operator==(const irr_ptr<T>& r) const { return ptr == r.ptr; }
		bool operator!=(const irr_ptr<T>& r) const { return ptr != r.ptr; }
		bool operator<(const irr_ptr<T>& r) const { return ptr < r.ptr; }

		irr_ptr& operator=(const irr_ptr& r)
		{
			ptr = r.ptr;
			ptr->grab();
			return *this;
		}
	};
}

#endif

A usage example:

Code: Select all

irr_ptr<IrrlichtDevice> device = createDevice(video::EDT_DIRECT3D9);

In this example, the created device will be dropped when "device" goes out of scope, unless of course "device" is assigned somewhere else. Also note the implicit constructor call.

[Sylence]

Nice work!
However I think there is a bug in the = operator.

You are passing r as a const reference but you change it in the function. Does this even compile ?

[SSG]

Fixed. I had the left and right sides confused. Thanks for that.

[Sylence]

Ah now the operator makes sense ^^
But shouldn't you call drop() on ptr before changin it?

If ptr is something valid and you override it the object will never be destroyed because there is the undropped grab from the constructor.

[rogerborg]

It's not as fancy as boost::shared_ptr, but it's good enough for the job.

How would you go about proving that?

[SSG]

We're programmers, not scientists. If we relied purely on emprical evidence we wouldn't have wars such as C vs C++, Irrlicht vs Ogre, Allegro vs SDL, etc. The code is there if you want it.

[hybrid]

Hmm, I'm out - I'm a scientist

[JP]

Woah that's someone who doesn't work in the programming industry then!

[grafikrobot]

Sorry to say but that code suffers from a variety of problems:

* It's not exception safe.
* It doesn't have safe bool operations.
* It doesn't have swap operations and hence would be suboptimal in std containers and algorithms.
* It's missing a get_pointer function accessor to make it work with things like Boost Bind.
* It doesn't have introspection typedefs for use with meta-programming, like bind, lambdas, and many other Boost and std algorithms need.
* It's missing a reset(). Although this might be considered optional, having it mirror std::auto_ptr and other smart ptrs is convenient.
* The example is flawed in that it's a memory leak because of the extra reference count create* functions have in Irrlicht. You would need to add a drop() after the construction/assignment.

[rogerborg]

We're programmers, not scientists.

"Prove" was perhaps a bit strong. How would you demonstrate that it does what it promises under all circumstances?

I ask because it's pretty important that a tool that's supposed to make memory management more robust actually be very robust itself. Otherwise users will just switch from debugging their own memory problems to debugging your memory problems.

Anyway, thanks for taking a stab at it. I'll keep checking back to see how this is progressing.

[CuteAlien]

copy-constructor does not check if it's getting a reference to itself.

[SSG]

Sorry to say but that code suffers from a variety of problems:

* It's not exception safe.
* It doesn't have safe bool operations.
* It doesn't have swap operations and hence would be suboptimal in std containers and algorithms.
* It's missing a get_pointer function accessor to make it work with things like Boost Bind.
* It doesn't have introspection typedefs for use with meta-programming, like bind, lambdas, and many other Boost and std algorithms need.
* It's missing a reset(). Although this might be considered optional, having it mirror std::auto_ptr and other smart ptrs is convenient.

That would go way beyond the scope of my requirements. If you want full boost compliance, feel free to add it yourself

* The example is flawed in that it's a memory leak because of the extra reference count create* functions have in Irrlicht. You would need to add a drop() after the construction/assignment.

Fixed. Thanks.

copy-constructor does not check if it's getting a reference to itself.

Suppose we have the following code, which I imagine is the scenario you're talking about:

Code: Select all

irr_ptr<A> a = createA(foo, bar);
a = irr_ptr<A>(a);

The following steps occur:

1. A new irr_ptr<A> is created, storing the pointer to an IReferenceCounted object (of which A is a descendant) with a reference count of 1, returned by createA(foo, bar).
2. Said object is assigned to a.
3. On line 2, a new irr_ptr<A> is created, using the IReferenceCounted pointer in a, incrementing its reference count by 1. So now the reference count is 2.
4. The new object is assigned to a.
5. The old object is no longer assigned to a, so its destructor is called which decrements the pointer's reference count. So now the reference count is 1.

This is why I don't check for the copy constructor getting a reference to itself.

I ask because it's pretty important that a tool that's supposed to make memory management more robust actually be very robust itself. Otherwise users will just switch from debugging their own memory problems to debugging your memory problems.

This risk could be mitigated simply by replacing IReferenceCounted with boost::shared_ptr in Irrlicht 1.6.

[grafikrobot]

Sorry to say but that code suffers from a variety of problems:

* It's not exception safe.
* It doesn't have safe bool operations.
* It doesn't have swap operations and hence would be suboptimal in std containers and algorithms.
* It's missing a get_pointer function accessor to make it work with things like Boost Bind.
* It doesn't have introspection typedefs for use with meta-programming, like bind, lambdas, and many other Boost and std algorithms need.
* It's missing a reset(). Although this might be considered optional, having it mirror std::auto_ptr and other smart ptrs is convenient.

That would go way beyond the scope of my requirements. If you want full boost compliance, feel free to add it yourself

I did in my own version of it, which I've had for close to a year now. And hence why I could rattle off the extra requirements by just looking at my code. But alas, it's not code I'm sharing.

This risk could be mitigated simply by replacing IReferenceCounted with boost::shared_ptr in Irrlicht 1.6.

That might not be the best option since shared_ptr is not a lightweight smart ptr. And you can use smart_ptr as it is with Irrlicht by just using a custom deleter and a pair of creation utility functions (also code I already use). But if/when Irrlicht starts living better with multi-threading smart_ptr might become a good option.

[vitek]

This is why I don't check for the copy constructor getting a reference to itself.

I'm sorry, but these guys are right. Your code is _full_ of holes. Imagine the following...

Code: Select all

irr_ptr<ISceneNode> a = smgr->addCubeSceneNode(10);
// what happens when a is destructed?

Or something like this...

Code: Select all

irr_ptr<IReferenceCounted> a(new DerivedReferenceCounted);
a = a;
a = a; 
a = a;

There are more...

Travis

[SSG]

Code: Select all

boost::shared_ptr<ISceneNode> a = smgr->addCubeSceneNode(10);
// what happens when a is destructed?

Fixed.

Code: Select all

boost::shared_ptr<IReferenceCounted> a(new DerivedReferenceCounted);
a = a;
a = a;
a = a;

Fixed.