Use unique_ptr/shared_ptr for objects_

[mspraggs]

This will eliminate memory leaks. Even though we do use new and delete, there may be paths of execution in the code that we can't see, which may result in memory that isn't properly deallocated. It's safest just to use a unique_ptr

Addresses #6

[Fyll]

If you can find a way to do this, I'd be more than happy with it. I only used ordinary pointers because I couldn't think how to do it with smart ones.

[mspraggs]

I'll have a stab and see how I get on.

[mspraggs]

I started doing this last night, and managed to get something that would compile. I ended up with a few bugs though, so I need to fix them.

Also now I completely see where you're coming from as regards not being able to do it. Normally player in game is an instance of Player, and you want to be able to add a Player* to objects. My solution is a tiny bit dirty, and I haven't merged in your AI code yet so I have no idea whether it'll work. What I did was to make player_ an Object*, then do this:

std::unique_ptr<Object> newPlayer = std::unique_ptr<Object>(new Player());
player_ = static_cast<Object*>(newPlayer.get());
world_.getCurrentRoom().addObject(std::move(newPlayer));

In Game::reset, it gets more dirty:

std::unique_ptr<Object> newPlayer = std::unique_ptr<Object>(new Player());
Object* pOldPlayer = player_;
player_ = static_cast<Object*>(newPlayer.get());
// Now replace the std::unique_ptr<Object> that has the pointer pOldPlayer.
world_.getCurrentRoom().replaceObject(pOldPlayer, std::move(newPlayer));

addObject now takes an rvalue reference to allow moving the non-copyable uniqueptr to the objects vector, so if you see std::unique_ptr<Object>&& obj that's what this means.

I've just merged your changes, and see you've made quite a few changes to Player etc., so I'll have another go at implementing this, and think of a way to do it more cleanly.

EDIT: The alternative is to use shared_ptr, which can be copied, but we may be able to get away without doing this. I'll have a look.

[Fyll]

First of all, is this worth the effort? Secondly, why are shared pointers bad?

What with me being an old code fogey, I'd suggest a union if you want to put the Player's pointer in with the other unique pointers. :P

...Actually, that'd probably work and be (relatively) clean.

[mspraggs]

It means we'll never have to worry about deallocating Objects. All these new features have been added to C++11 for the programmer's convenience, so we might as well use them.

Shared pointers aren't bad, per se. They basically use reference counting to keep track of how many shared_ptrs point to a particular object at any one time. When this counter hits zero, the object is destroyed. Basically you use them when you need two pointers, and you're not sure which pointer will go out of scope first. EDIT: General advice is to use unique_ptr when you can, and you know that an object's lifetime is the lifetime of that pointer.

It would work, but it seems a bit like overkill for the sake of just one object. However, it may be useful given there's the remote chance that if the std::unique_ptr<Object> referencing player is destroyed, whilst player_ remains the same. In this case player_ would no longer be valid and there'd be a spectacular crash.

If we used shared_ptr, then we could just call objects_.clear() and push_back the player_ into objects_, without player_ being destroyed.

I think I've pretty much talked myself round to using shared_ptr. Perhaps that's the way to go.

[mspraggs]

Ok I've implemented this now using shared_ptr. In particular I've added a function to Room, replaceObject, which takes two sharedptrs and replaces the first with the second, returning the second. This is useful when reallocating player in Game::reset