irr_ptr

[CuteAlien]

@vitek, wow thanks, that's cool. I haven't seen that before. Do you maybe also know why boost uses that instead of overloading operator bool()?

[vitek]

See Safe Bool Idiom.

Travis

[CuteAlien]

See Safe Bool Idiom.

Making stuff easy in c++ is hard.

Thanks again. Newest version kicked the valid() and replaced it with a safebool.

[vitek]

If your design intent is to handle pointers that may or may not be need to be dropped, I think that the set_created() and operator=(T*) functions should be removed, and the default grab value should be removed from the constructor.

It is entirely to easy to use operator=() with a pointer from a create...() function, and that would lead to a memory leak. If you used the above suggestions, the user would have to write something like the following...

Code: Select all

IrrPtr<IImage> p = IrrPtr<IImage>(driver->createImage(...), false);

It is pretty verbose, but it requires the user to be consious of the behavior of the pointer type. It also makes usage consistent, so handling a pointer that is from an add...() call is pretty much the same as it would be for a create...() call.

Another thing you could do would be to create helpers to handle this explicitly...

Code: Select all

template <class T>
IrrPtr<T> make_created (T* ptr)
{
  return IrrPtr<T>(ptr, false);
}

template <class T>
IrrPtr<T> make_added (T* ptr)
{
  return IrrPtr<T>(ptr, true);
}

// use it like this
IrrPtr<ISceneNode> p = make_added(smgr->addCubeSceneNode(...));
IrrPtr<IImage> q = make_created(driver->createImage...(...));

Another improvement would be to handle implicit conversions...

Code: Select all

IrrPtr<IMeshSceneNode> msn(smgr->addMeshSceneNode(), true);

// these won't compile
IrrPtr<ISceneNode> sn(msn);
sn = msn;

// of course this will, but it exposes the fact that the pointer is different from a regular raw pointer
IrrPtr<ISceneNode> sn(msn.get(), true);
sn = IrrPtr<ISceneNode>(msn.get(), true);

Travis

[CuteAlien]

If your design intent is to handle pointers that may or may not be need to be dropped, I think that the set_created() and operator=(T*) functions should be removed, and the default grab value should be removed from the constructor.

The original intent was to handle pointers that would do the whole grab()/drop() lifecycle for the refcounted Irrlicht pointers. That's why I didn't give created (already grab()'ed) pointers equal rights, but did see them as exception. I already got a bad feeling about the operator=(T*), so I will probably drop that. I'll have to think some more about giving grab()'ed and non-grab()'ed pointers equal rights.

About the IrrPtr<IImage> vs. make_created vs. set_created vs. set(pointer, grab): If I give those pointers equal rights then I have to change it, as the current design makes them unequal on purpose. I'm not that much of a fan of the explicit version, as I'm not sure if the names are significant enough to have them outside a class-space. Would you see anything that speaks against a setter with grab as parameter (not default-parameter)?

Another improvement would be to handle implicit conversions...

Yeah, I know. I just didn't find time yet to look up which traps c++ will put in my way when trying that. I think there will be some, because boost has those strange casts like:

Code: Select all

class IBase{};
class Derived : public IBase {}
typedef boost::shared_ptr<IBase> base_ptr;
typedef boost::shared_ptr<Derived> derived_ptr;
base_ptr b(new Derived());
derived_ptr foo(boost::shared_static_cast<Derived>(b));	// using static_cast
// derived_ptr foo(boost::shared_dynamic_cast<Derived>(b));	// using dynamic_cast
// derived_ptr foo(boost::shared_polymorphic_downcast<Derived>(b));	// using dynamic_cast unless NDEBUG is set - then static_cast

[vitek]

Yeah, you could add a set() or assign() function that takes a T* and a bool. You could also make the previously named functions members, but then you lose the ability for the compiler to deduce the template argument type.

I'm not saying you have to do anything, I'm just bringing up the issues as I see them and offering workarounds. :)

Travis

[CuteAlien]

I'm not saying you have to do anything, I'm just bringing up the issues as I see them and offering workarounds. :)
Travis

I'm appreciating it :-)

Removing the operator= for direct pointer assignment was definitely a good idea - I messed that already up at one place in my code! I can't bring myself yet to remove the default parameter for grab, as it gives some hint for the way the class is intended to be used.