Является ли использование векторов указателей здесь ненужным или хуже, вызывает утечки памяти?

Question

Я относительно новичок в программировании на C ++, но я программист на C 10 лет, поэтому мне удобнее использовать указатели на объекты, чем ссылки на объекты.

Я пишу пасьянс - этот дизайн небезопасен? Есть ли лучший способ?

Во всяком случае, у меня есть класс SolitaireGame:

class SolitaireGame:
{
    public:
        SolitaireGame( int numsuits = 1 );
    private:
        Deck * _deck;
        vector<Card> _shoe;
};

Deck определяется следующим образом:

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;
};

Конструктор Deck берет ссылку на вектор из Card объектов (туфель, обычно 104 карты) и помещает указатель на каждую карту на собственную деку указателей.

Deck::Deck( vector<Card>& shoe )
{
    vector<Card>::iterator iter = shoe.begin();

    while( iter != shoe.end() )
    {
        _cards.push_front( &(*iter) );
        iter++;
    }
}

}

Обувь создана в конструкторе SolitaireGame. Как только этот вектор динамически созданных Card объектов был создан - я передаю ссылку на этот вектор конструктору.

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 );
}

Моя идея заключалась в том, что башмак будет контейнером для фактической памяти для объектов Card, а Deck и Columns просто обрабатывают указатели на эти Card объекты.

Answer 1

Просто взяв этот фрагмент кода, вы теряете динамически созданные карты.

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 ) добавляет копию объекта Card, на который указывает c, к вектору Card с. После этого вам не удастся удалить исходный Card, созданный ранее в строке.

Выделение вектора NUM_CARDS_IN_SHOE Cards может быть достигнуто гораздо проще, например:

std::vector<Card> _shoe( NUM_CARDS_IN_SHOE );

Если посмотреть на структуру вашей карты, похоже, что вы обладаете (или почти имеете) строгим владением объектами, поэтому я не думаю, что вам нужно динамически создавать Card s.

Обратите внимание, что ваша локальная переменная _shoe скрывает переменную класса _shoe. Это, вероятно, не то, что вам нужно, так как локальный _shoe, который вы передаете конструктору Deck, выйдет из области видимости в конце конструктора.

Если вы переупорядочиваете переменные в SolitaireGame, вы, вероятно, можете сделать что-то вроде этого:

class SolitaireGame:
{
public:
    SolitaireGame( int numsuits = 1 );
private:
    vector<Card> _shoe;
    Deck _deck;
};

SolitaireGame::SolitaireGame( int numsuits )
    : _shoe(NUM_CARDS_IN_SHOE)
    , _deck(_shoe)
{
}

Я изменил _deck с указателя. Я использую тот факт, что переменные-члены создаются в порядке, объявленном в определении класса, поэтому _shoe будет полностью создан, прежде чем он будет передан в качестве ссылки на конструктор для _deck. Преимущество этого состоит в том, что я избавил от необходимости динамически выделять _deck. Без использования new я знаю, что не могу пропустить звонки на delete, так как ничего не нужно явно освобождать.

Вы правы, что вы можете хранить указатели на Card в _shoe в вашем _deck без каких-либо проблем с управлением памятью, но учтите, что вы не должны добавлять или удалять любые из Card в _shoe в течение жизни игры, иначе вы лишите законной силы все указатели в _deck.

Answer 2

Я думаю, что есть две ошибки:

1. Когда вы делаете _shoe.push_back( *c );, вы создаете копию объекта Card, поэтому память, зарезервированная для c, никогда не будет освобождена. Кстати, вы всегда должны проверять, что для каждого нового существует дополнительное удаление. Где твое удаление?
2. В конструкторе Deck вы сохраняете указатели на объекты, которые находятся в стеке (vector<Card> _shoe;), поэтому, как только закончится конструктор SolitaireGame, они будут удалены, а ваши указатели станут недействительными , РЕДАКТИРОВАТЬ: я вижу, что у вас есть еще одна _shoe в вашем классе, поэтому нет необходимости объявлять другую локальную переменную _shoe, фактически, просто не объявив ее, вы решите эту проблему.

Надеюсь, это вам немного поможет.

Answer 3

Я не думаю, что ключевое слово new должно присутствовать где-либо в коде этих классов, и я не понимаю, зачем вам приходилось сталкиваться с проблемой обмена картами с помощью указателей. Хранение адресов элементов, содержащихся в векторе, - это путь к катастрофе - вам нужно гарантировать, что после взятия адресов не будет никаких изменений в векторе, поскольку он имеет тенденцию перемещать вещи в памяти, не сообщая вам.

Предполагая, что объект Card не хранит ничего, кроме одной или двух целых, было бы намного проще работать с копиями и значениями.

_deck = new Deck( _shoe );

Опять же, я не вижу ни малейшей причины увеличить сложность программы, динамически выделяя объект, содержащий две целые и одну деку.

Если вас беспокоит стоимость копирования некоторых более крупных классов, которые у вас есть (которые, я бы оценил, не окажет здесь никакого влияния на воспринимаемую производительность), то просто не копируйте их, а передавайте их const ссылка (если вам не нужно мутировать экземпляр) или неконстантная ссылка / указатель в противном случае.

Answer 4

Начальные мысли:

1. В классе SolitaireGame вы объявляете _shoe как:

   vector<Card> _shoe;

   но в конструкторе вы помещаете в него кучу объектов следующим образом:

   c = new Card();

   _shoe.push_back( *c );

   Итак, вам нужно объявить это так:

   vector<Card*> _shoe;

2. Вы не инициализируете переменные в конструкторах, таких как deal_left и num_each_deal, в классе Deck. Я предполагаю, что вы оставили это, чтобы не загромождать код, но это хорошая идея.

3. Класс SolitaireGame создает и владеет объектами Deck. Он также имеет колоду с указателями на объекты карты SolitaireGame. Владение здесь неясно - кто их удалил? Хотя указатели на объекты в нескольких контейнерах будут работать, это может усложнить отладку, поскольку есть возможность многократного удаления, использования после его удаления, утечек и т. Д. Возможно, дизайн можно упростить. Возможно, изначально у Deck есть объекты Card, а когда они удалены, они помещаются в вектор в SolitaireGame и не существуют в обоих одновременно.

4. В конструкторе SolitaireGame вы объявляете еще один вектор карточек, который затеняет тот, который объявлен в объявлении класса. Когда вы помещаете в нее объекты Card, они не будут перемещены в правильный вектор, который выйдет из области видимости в конце конструктора, и ваш член класса будет пустым. Просто избавься от конструктора.

В любом случае, мне нужна чашка чая. После этого я еще раз посмотрю и смогу предложить что-нибудь еще.

Answer 5

Эта программа будет пропускать память, Хотите узнать, почему? или как ?

push_back

Помните, этот вызов не вставляет предоставленный элемент, но создает его копию для собственного использования. Прочитайте это для деталей

Итак

    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 

Скопируйте приведенную ниже программу и выполните ее, {я просто добавил ведение журнала}, попробуйте оба варианта выше

#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;
}

Answer 6

Поскольку у такого игрового объекта всегда есть своя колода, вы должны рассмотреть возможность сделать объект Deck реальным членом внутри SolitairGame, а не просто указателем. Это значительно упростит управление объектом колоды в течение всей жизни. Например, вам больше не понадобится пользовательский деструктор. Имейте в виду, что контейнеры STL содержат копии. Если вы напишите что-то вроде

myvector.push_back(*(new foo));

у вас утечка памяти.

Кроме того, хранение указателей на элементы вектора опасно, поскольку указатели (или итераторы в целом) могут стать недействительными. Для вектора это тот случай, когда ему нужно расти. Альтернативой является std :: list, который сохраняет действительность итераторов после вставки, удаления и т. Д.

Кроме того, имейте в виду, что в C ++ структуры и классы обычно получают неявные конструкторы копирования и операторы присваивания. Уважайте правило трех . Либо запретите копирование и назначение, либо убедитесь, что ресурсы (включая динамически выделенную память) правильно управляются.