Я правильно использую удаление здесь? - PullRequest
Новая
       10

Я правильно использую удаление здесь?

1 голос
/ 03 марта 2010

Я только начал совмещать свои знания классов C ++ и динамических массивов. Мне дали совет, что «всякий раз, когда я использую новый оператор», я должен удалить. Я также знаю, как работают деструкторы, поэтому я думаю, что этот код правильный:

main.cpp 

...
int main()
{
    PicLib *lib = new PicLib;
    beginStorage(lib);
    return 0;
}

void beginStorage(PicLib *lib)
{
...
    if (command != 'q')
    {
        //let's assume I add a whole bunch
            //of stuff to PicLib and have some fun here
        beginStorage(lib);
    }
    else
    {
        delete lib;
        lib = NULL;
        cout << "Ciao" << endl;
    }
}

PicLib.cpp 

...

PicLib::PicLib()
{
    database = new Pic[MAX_DATABASE];
    num_pics = 0;
}

PicLib::~PicLib()
{
    delete[] database;
    database = NULL;
    num_pics = 0;
}
...

Я заполняю свой PicLib классом Pic, содержащим больше динамических массивов. Деструктор Pic удаляет их так же, как показано выше. Я думаю, что delete [] database избавляется от всех этих классов должным образом.

Так необходимо ли удаление в main.cpp ? Здесь все выглядит неуклюжим Дори?

Ответы [ 8 ]

5 голосов
/ 03 марта 2010

Есть несколько проблем: 

int main() 
{ 
  PicLib *lib = new PicLib; 
  beginStorage(lib); 
  return 0; 
}

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

Но в этом случае просто объявите это локально (и передайте по ссылке): 

int main() 
{ 
    PicLib  lib; 
    beginStorage(lib); 
    return 0; 
}

В beginStorage ()

Но я не вижу причин манипулировать указателем. Передайте его по ссылке и просто используйте его локально. 

void beginStorage(PicLib& lib)
{
 ....
}

В классе PicLib у вас есть указатель RAW: базы данных.

Если у вас есть указатель RAW, которым вы владеете (вы создаете и уничтожаете его), вы должны переопределить сгенерированные компилятором версии конструктора копирования и оператора присваивания. Но в этом случае я не вижу причин использовать указатель, было бы проще просто использовать вектор: 

class PivLib
{
    private:
        std::vector<Pic>   databases;
};
3 голосов
/ 03 марта 2010

Да, все, что вы создаете с помощью new , должно быть удалено с помощью delete , а все, что создано с помощью new [] необходимо удалить с помощью delete [] , в какой-то момент.

В вашем коде я бы указал на некоторые вещи:

  • Предпочитайте std :: vector <>, используя new [] и delete []. Это сделает управление памятью за вас. Также обратите внимание на умные указатели, такие как auto_ptr, shared_ptr и C ++ 0x unique_ptr для автоматического управления памятью. Это поможет вам забыть об удалении и вызвать утечку памяти. Если вы можете, вообще не используйте new / new [] / delete / delete []!
  • Если вам нужно что-то новое и что-то удалить, очень хорошая идея для нового в конструкторе класса и удалить в деструкторе класса. Когда генерируются исключения, объекты выходят из области видимости и т. Д., Их деструкторы вызываются автоматически, что помогает предотвратить утечки памяти. Это называется RAII.
  • Удаление beginStorage его параметра - потенциально плохая идея. Может произойти сбой, если вы вызовете функцию с указателем, не созданным с помощью new , потому что вы не можете удалить любой указатель. Было бы лучше, если бы main () создал PicLib в стеке, а не использовал new и delete, а для beginStorage взять ссылку и ничего не удалять.
2 голосов
/ 03 марта 2010

Необходимо удалить файл main.cpp.

Вероятно, это вопрос личных предпочтений, но я бы не советовал вызывать new и delete в отдельных логических частях (здесь вызов delete для экземпляра PicLib находится в отдельной функции). Обычно лучше, чтобы ответственность за распределение и освобождение была отдана только одной части.

@ AshleysBrain предлагает лучшее предложение (о создании стека PicLib), хотя это может вызвать проблемы, если PicLib занимает слишком много памяти.

2 голосов
/ 03 марта 2010

else не идет с while. Вы бы хотели что-то вроде: 

void beginStorage(PicLib *lib)  
{  
    while (command != 'q') 
    { 
        //let's assume I add a whole bunch 
            //of stuff to PicLib and have some fun here 
    } 

    delete lib; 
    lib = NULL;  // Setting to NULL is not necessary in this case,
                 // you're changing a local variable that is about
                 // to go out of scope.
    cout << "Ciao" << endl;
}

delete выглядит хорошо, но вы должны убедиться, что документ beginStorage становится владельцем объекта PicLib. Таким образом, любой, кто использует beginStorage, знает, что ему не нужно удалять его позже.

2 голосов
/ 03 марта 2010

Да, это необходимо, если вы не используете auto_ptr (и не читаете семантику auto_ptr, прежде чем использовать его - вы не можете скопировать его вокруг).

например: 

int main()
{
    auto_ptr<PicLib> lib = new PicLib;
    beginStorage(lib);
    return 0;
} // auto_ptr goes out of scope and cleans up for you
1 голос
/ 03 марта 2010

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

0 голосов
/ 03 марта 2010

Да, иначе деструктор для PicLib не будет вызван.

Одно стилистическое примечание: если вы добавляете объект области метода в функцию, попробуйте удалить ее в той же функции. Будучи младшим инженером, которому приходилось проходить крупные проекты по устранению утечек памяти других людей ... это намного облегчает чтение других людей Судя по всему, вы можете удалить * lib после возвращения beginStorage (). Тогда было бы легче увидеть область действия * lib в одном месте.

0 голосов
/ 03 марта 2010

Все выглядит хорошо.

Удаление в main.cpp необходимо, потому что если вы не вызывали delete, то деструктор не запускается и ваш массив не удаляется. Вы также будете пропускать память для класса, а не только для массива.

...