Is using vectors of pointers unnecessary here, or worse, cause of memory leaks?
I'm relatively new to C ++ programming, but I've been a C programmer for 10 years, so I'm more comfortable with object pointers than object references.
I am writing a Solitaire game - is this design insecure? Is there a better way?
Anyway, I have a class SolitaireGame
:
class SolitaireGame:
{
public:
SolitaireGame( int numsuits = 1 );
private:
Deck * _deck;
vector<Card> _shoe;
};
Deck
is defined this way:
class Deck:
{
public:
Deck::Deck( vector<Card>& shoe );
~Deck();
int DealsLeft() const { return deals_left; }
Card * PullCard();
private:
int deals_left;
int num_each_deal;
deque<Card *> _cards;
};
The constructor Deck
takes a reference to a vector of objects Card
(boot, usually 104 cards) and pushes the pointer on each card onto its own pointer of pointers.
Deck::Deck( vector<Card>& shoe )
{
vector<Card>::iterator iter = shoe.begin();
while( iter != shoe.end() )
{
_cards.push_front( &(*iter) );
iter++;
}
}
}
Shoes are created in the constructor SolitaireGame
. Once this vector of dynamically created objects has Card
been created, I pass a reference to this vector to the constructor.
SolitaireGame::SolitaireGame( int numsuits ):_numsuits(numsuits )
{
Card * c;
vector<Card> _shoe;
for( int i = 0; i < NUM_CARDS_IN_SHOE; i++ )
{
c = new Card();
_shoe.push_back( *c );
}
_deck = new Deck( _shoe );
}
My idea was that the boot would be the container for the actual memory for the objects Card
, and the tags Deck
and are Columns
just meant for those objects Card
.
source to share
By simply using this piece of code, you are leaking dynamically generated maps.
Card * c;
vector<Card> _shoe;
for( int i = 0; i < NUM_CARDS_IN_SHOE; i++ )
{
c = new Card();
_shoe.push_back( *c );
}
_shoe.push_back( *c )
appends a copy of the object Card
it points c
to to the vector Card
s. Then you don't delete the original Card
one created in the line earlier.
Highlighting a vector NUM_CARDS_IN_SHOE
Cards
can be much easier:
std::vector<Card> _shoe( NUM_CARDS_IN_SHOE );
Looking at the structure of your map, it looks like you have (or nearly) strong ownership between objects, so I don't think you need to dynamically create your Card
s.
Note that your local variable is _shoe
shadowing your class variable _shoe
. This is probably not what you want, since the local _shoe
you pass to the constructor Deck
will go out of scope at the end of the constructor.
If you are reordering variables in SolitaireGame
, you can do something like this:
class SolitaireGame:
{
public:
SolitaireGame( int numsuits = 1 );
private:
vector<Card> _shoe;
Deck _deck;
};
SolitaireGame::SolitaireGame( int numsuits )
: _shoe(NUM_CARDS_IN_SHOE)
, _deck(_shoe)
{
}
I changed _deck
as a pointer. I take advantage of the fact that member variables are built in the order declared in the class definition, so it _shoe
will be fully constructed before being passed as a constructor reference for _deck
. The advantage of this is that I have eliminated the need for dynamic selection _deck
. Without using, new
I know that I cannot have missed calls on delete
since nothing needs to be explicitly released.
You're right that you can store pointers Card
in _shoe
in _deck
without any problems with the management of memory, but note that you should not add or remove any of the Card
in _shoe
during the life of the game otherwise you will cancel all pointers _deck
.
source to share
I think there are two mistakes:
- When you do
_shoe.push_back( *c );
, you create a copy of the Card object, so the memory reserved for c will never be freed. By the way, you should always check that there is an additional delete for every new one. Where do you delete? - In the Deck constructor, you keep pointers to the objects that are on the stack (
vector<Card> _shoe;
), so once the SolitaireGame constructor finishes, they will be removed and your pointers will be invalid. EDIT: I see you have another _shoe in your class, so there is no need to declare another local variable _shoe, just by not declaring it will solve this problem.
Hope this helps you a little.
source to share
Initial thoughts:
-
In the SolitaireGame class, you declare _shoe as:
vector<Card> _shoe;
but in the constructor you push the heap objects to it like this:
c = new Card();
_shoe.push_back( *c );
So, you need to declare it like this:
vector<Card*> _shoe;
-
You don't initialize variables in constructors like deal_left and num_each_deal in the Deck class. I assume you left it to keep your code from cluttering, but it's a good idea.
-
The SolitaireGame class creates and owns Deck objects. It also has a deck with pointers to SolitaireGame map objects. The ownership here is unclear - who deleted them? While pointers to objects in multiple containers will work, it can be difficult to debug as there is a possibility for multiple deletions, use after deletion, leaks, etc. Perhaps the design could be simplified. It is possible that the Bundle has its own Map objects initially, and when they are deleted, they end up in the vector in the SolitaireGame and do not exist in both at the same time.
-
In the constructor of SolitaireGame, you declare another vector of maps that obscures the declaration declared in the class declaration. When you click on it the Map objects will not fall into the correct vector, which will go out of scope at the end of the constructor and your class member will be empty. Just get rid of it from the constructor.
Anyway, I need a cup of tea. After that, I'll take another look and see if I can suggest anything else.
source to share
I don’t think the new keyword should appear anywhere in the code of these classes, and I don’t understand why you are facing the problem of sharing cards through pointers. Storing the addresses of elements stored in a vector is a recipe for disaster - you need to ensure that after you take the addresses, there is no change in the vector, as it tends to move things around in memory without telling you.
Assuming the Card object does not store anything other than one or two int, it would be much easier to work with copies and values.
_deck = new Deck( _shoe );
Again, I see no reason to increase the complexity of the program by dynamically allocating an object containing two ints and a deque.
If you're worried about the cost of copying some of the larger classes you have (which I would appreciate, it has zero perceived performance impact here), then just don't copy them and pass them const (unless you need to mutate the instance). otherwise no reference is specified / pointer.
source to share
Since such a game object always has its own deck, you should consider making the Deck object a real member within the SolitairGame - not just a pointer. This will greatly simplify the management of the deck object throughout its entire lifecycle. For example, you no longer need a custom destructor. Be aware that STL containers contain copies. If you write something like
myvector.push_back(*(new foo));
you have a memory leak.
Also, storing pointers to elements of a vector is dangerous because pointers (or iterators in general) can become invalid. For a vector, this is the case when it should grow. An alternative is std :: list, which keeps iterators valid after insertion, deletion, etc.
Also, keep in mind that in C ++, structs and classes usually receive implicit copy constructors and assignment operators. Read the rule of three . Either disallow copying and assignment, or ensure that resources (including dynamically allocated memory) are properly managed.
source to share
This program will leak memory, Want to know why? or how?
push_back
Remember that this call does not insert your incoming element, but creates a copy of it for your own use. Read this one for details
So
Card *c = new Card(); // This is on heap , Need explicit release by user
If you change it to
Card c; // This is on stack, will be release with stack unwinding
Copy below program and execute it, {I just added an entry}, try both options above
#include<iostream>
#include <vector>
#include <deque>
using namespace std;
const int NUM_CARDS_IN_SHOE=120;
class Card
{
public:
Card()
{
++ctr;
cout<<"C'tor callend: "<<ctr<<" , time"<<endl;
}
~Card()
{
++dtr;
cout<<"D'tor called"<<dtr<<" , time, num still to release: "<<((ctr+cpy)-dtr)<<endl;
}
Card& operator=(const Card & rObj)
{
return *this;
}
Card (const Card& rObj)
{
++cpy;
cout<<"Cpy'tor called"<<cpy<<endl;
}
private:
static int ctr,dtr,rest,cpy;
};
int Card::ctr;
int Card::dtr;
int Card::rest;
int Card::cpy;
class Deck
{
public:
Deck::Deck( vector<Card>& shoe );
~Deck();
int DealsLeft() const { return deals_left; }
Card * PullCard();
private:
int deals_left;
int num_each_deal;
std::deque<Card *> _cards;
};
Deck::Deck( vector<Card>& shoe )
{
vector<Card>::iterator iter = shoe.begin();
while( iter != shoe.end() )
{
_cards.push_front( &(*iter) );
iter++;
}
}
class SolitaireGame
{
public:
SolitaireGame( int numsuits = 1 );
private:
Deck * _deck;
std::vector<Card> _shoe;
};
SolitaireGame::SolitaireGame( int numsuits )
{
Card * c;
vector<Card> _shoe;
for( int i = 0; i < numsuits; i++ )
{
c = new Card();
_shoe.push_back( *c );
}
_deck = new Deck( _shoe );
}
int main()
{
{
SolitaireGame obj(10);
}
int a;
cin>>a;
return 0;
}
source to share