Deleting pointer and re-initializing

[lockandstrike]

So I'm dealing with a doubt. I have a struct:

struct Card{
	Card(std::string tFile, int _damage, int _life);
	void draw(sf::RenderWindow &App);
	sf::Sprite sCard;
	sf::Texture tCard;
	int damage;
	int life;
};

I then have further on a class Deck that is basically a vector of the struct Card. Because I'm reading this from a file I require a temporary Card that is pushed back onto the vector and then cleared. To do so I thought of declaring it as class member through a pointer. I did that to take advantage of the ponter = new Card() function. But I have to clear it so here is my idea. I delete the pointer and re-initialize it as such:

Card *tempCard;

...

tempCard = new Card(parameters here);

...

delete tempCard;
tempCard = new Card(parameters here);

Would this work and would it be safe from memory leaks

[MrHutch]

So it's a vector of pointers to the struct? Do you loop through the file when reading it?

Can't you just do...

deck.push_back( new Card( /*params */ );

Then have the deck free up the memory in its destructor.

[Catfish666]

Would this work and would it be safe from memory leaks

In theory, if nothing throws an exception, then yes.

int *p = new int[100];

// ...
// exception thrown here somewhere
// ...

delete[] p; // this is never reached and memory is leaked 

Here's an illustration of the above:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43

#include <iostream> class Whine { public: Whine() { std::clog << "I'm alive @ " << this << '\n'; } ~Whine() { std::clog << "I'm dead @ " << this << '\n'; } }; void evil_function() { throw "I'm very evil."; } void innocent_function() { Whine w; Whine *pw = new Whine; std::clog << "before calling evil_function()\n"; evil_function(); // nothing beyond here gets done std::clog << "after calling evil_function()\n"; delete pw; } int main() try { innocent_function(); } catch (const char *s) { std::clog << "Caught an exception: " << s << '\n'; }

I'm alive @ 0x28feeb I'm alive @ 0x3f0f48 before calling evil_function() I'm dead @ 0x28feeb Caught an exception: I'm very evil.

Notice how the local variable w gets destroyed properly, because it was created on the stack, and not allocated on the heap with new. And as expected, pw is never deleted.

To do away with all of this, you should use smart pointers.
In this case, as in most, std::unique_ptr is an excellent choice.

If your C++ compiler standard library doesn't implement it, use std::auto_ptr but remember it was deprecated.

http://www.cplusplus.com/reference/memory/unique_ptr/
http://www.cplusplus.com/reference/memory/auto_ptr/

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43

#include <iostream> #include <memory> class Whine { public: Whine() { std::clog << "I'm alive @ " << this << '\n'; } ~Whine() { std::clog << "I'm dead @ " << this << '\n'; } }; void evil_function() { throw "I'm very evil."; } void innocent_function() { Whine w; std::unique_ptr<Whine> spw(new Whine); std::clog << "before calling evil_function()\n"; evil_function(); // nothing beyond here gets done std::clog << "after calling evil_function()\n"; } int main() try { innocent_function(); } catch (const char *s) { std::clog << "Caught an exception: " << s << '\n'; }

I'm alive @ 0x28feef I'm alive @ 0x350f48 before calling evil_function() I'm dead @ 0x350f48 I'm dead @ 0x28feef Caught an exception: I'm very evil.

[Catfish666]

Sorry for the double-post but I feel it's necessary, seen as I got a bit carried away with the previous one.

Reading the original post again, I'm not sure if smart pointers are the right way to go, especially if they were to end up with a vector of smart pointers.

Because I'm reading this from a file I require a temporary Card that is pushed back onto the vector and then cleared.

Do you really need to use dynamic memory allocation at all in this case?
I may be wrong of course, but I really don't think you do.

I'd appreciate it if you'd clarify whether you use an std::vector<Card>, or an std::vector<Card *>.

[lockandstrike]

It is a std::vector<Card>.

[Catfish666]

Well then, by adapting the code suggested by iHutch105, you could do:

deck.push_back(Card(/* parameters here */));

[lockandstrike]

ok. Thank you very much. I was over-thinking because I was already writing a clear() function and complicating my code and probably slowing it down. Anyway lots of thanks.

Best Regards,
jonhy31

[Disch]

Alert alert alert.


You have a subtle and fatal flaw with this vector.

sf::Sprites do not own their own texture. Instead, they hang on to a pointer of an existing texture. I assume that texture is 'tCard'.

This means you should not put this struct as an object in a vector because when the vector resizes, it might move the object to a different block in memory. If that happens, your sprite will have a pointer to a texture that no longer exists (it has been moved).

So every time you push_back or resize() this vector... all of your sprites have potential to break.

You can solve this one of several ways.

option 1) Do not put the sf::Texture in the Card struct... and instead put it in some other kind of resource manager (you should not have a 1:1 ratio of sprites to textures anyway, as this can often lead to duplicated textures being loaded).

option 2) Instead of a vector, use a container that guarantees it won't move elements around on resize (like std::list). Though depending on how you're using this vector, that may not be possible/practical.

option 3) Instead of having the sf::Texture as an object in the Card struct, make it a [smart] pointer in the card struct:

std::unique_ptr<sf::Texture>  tCard;

Now even if the card moves... the actual texture will not because it is allocated elsewhere on the heap.


option 4) Same idea as option 3.... but instead of pointer-izing the texture, pointerize the entire Card within the vector:

// bad
// std::vector<Card>  v;
// v.push_back( Card(1,2,3) );

// good
std::vector< std::unique_ptr<Card> > v;
v.emplace_back( new Card(1,2,3) );

EDIT:

Also, I highly recommend you use smart pointers whenever you do anything with new. If you are deleting things manually, you're probably doing it wrong.

std::unique_ptr<Card> p( new Card );

// no need to delete the card... it will be automatically deleted 

RAII and smart pointers are your friend. They make code easier and safer. Get familiar with them asap.

[Catfish666]

@ Disch: I used to think that putting smart pointers in containers is generally a bad idea.

In this case, wouldn't everything break just the same, if the OP ever made the mistake of passing one element of the std::vector<std::unique_ptr<Card>> by value instead of by reference?

Which is why in such cases std::shared_ptr would be more appropriate?

Edit: the copy constructor of std::unique_ptr is marked as deleted so that won't happen.

[lockandstrike]

Okay. So do I follow Disch's option with unique ptr?

[Disch]

Catfish666 wrote:

@ Disch: I used to think that putting smart pointers in containers is generally a bad idea.

I'm glad to hear that sentence in past tense. ;P

Putting auto_ptr in containers was a bad idea (because it didn't work), but C++11 smart pointers are much, much better.

Which is why in such cases std::shared_ptr would be more appropriate?

shared_ptr is for shared ownership. Shared ownership complicates design and is often unnecessary. It's much easier to have a single clear owner for an object.

Here, the owner would be the vector. When the object is in that vector, it's alive. As soon as you pop_back/erase/clear it... the object dies. Simple.

vector<unique_ptr<Card>> has all the same lifetime and scope rules of vector<Card>... the RAII means that it's all but impossible to leak memory... and it ensures that the texture will not move around in memory, which means your sprites will not go bad. It's win/win/win.

The only downside is there's an added level of indirection when accessing the card, but the impact that will make on this program is minimal/nonexistent.

jonhy31 wrote:

Okay. So do I follow Disch's option with unique ptr?

If you're asking me, I say yes (obviously).

Again... smart pointers are a critical part of modern C++. The sooner you get familiar with them the better.

You need dynamic allocation here to avoid the Sprite/Texture problem... and dynamic allocation without a smart pointer is difficult and error prone.

[lockandstrike]

So just to make this clear I have a temp unique pointer and a vector of unique ptr of the Card. Then i have to emplace_back the temp ptr into the deck. Correct?

[Disch]

You don't need a temp unique_ptr. Just put the ptr into the vector directly with emplace_back:

std::vector<std::unique_ptr<Card>> v;

// to add a new card:
v.emplace_back( new Card( "foo", 1, 2 ) );

If you want to make a temp unique_ptr first you can, but then you have to move it into the vector:

// make a temp pointer
std::unique_ptr<Card> tmp( new Card("bar",2,3) );

// move it into the vector
v.emplace_back( std::move(tmp) );

// note:  here... 'tmp' is no longer valid (it will be a null ptr) 

[lockandstrike]

Ok. Thank you very much for explaining me. After I finish this project I'm gonna throw my head into smart pointers.