C++ будет ли эта функция протекать?

Question

Я начал писать простую консольную игру Yahtzee для практики. У меня просто вопрос о том, будет ли эта функция утечка памяти. Функция roll вызывается каждый раз, когда кубики необходимо повторно прокатывать.

Он создает динамический массив. В первый раз он будет использовать 5 случайных значений. Для следующего прогона он будет только повторно свертывать все, за исключением костей, которые вы хотите сохранить. У меня есть еще одна функция для этого, но так как это не относится к этому вопросу, который я оставил его

Основная функция

int *kast = NULL;   //rolled dice 
int *keep_dice = NULL; //which dice to re-roll or keep 

kast = roll(kast, keep_dice); 
delete[] kast; 

и вот функция

int *roll(int *dice, int *keep) { 

    srand((unsigned)time(0)); 
    int *arr = new int[DICE]; 
    if(!dice) 
    { 
     for(int i=0;i<DICE;i++) 
     { 

      arr[i] = (rand()%6)+1; 
      cout << arr[i] << " "; 
     } 
    } 
    else 
    { 
     for(int i=0;i<DICE;i++) 
     { 
      if(!keep[i]) 
      { 
       dice[i] = (rand()%6)+1; 
       cout << "Change "; 
      } 
      else 
      { 
       keep[i] = 0; 
       cout << "Keep "; 
      } 
     } 
     cout << endl; 
     delete[] arr; 
     arr = NULL; 
     arr = dice; 

    } 
    return arr; 
} 

Answer 1

Да, это может привести к утечке. Например, использование cout может вызвать исключение, и если это произойдет, то ваш delete никогда не будет вызван.

Вместо распределения динамического массива самостоятельно, вы можете рассмотреть возможность возврата std::vector. Еще лучше, превратите вашу функцию в правильный алгоритм, который принимает итератор (в данном случае, back_insert_iterator) и записывает его вывод там.

Редактировать: Глядя на нее более тщательно, я считаю необходимым указать, что мне действительно не нравится основная структура этого кода. У вас есть одна функция, которая действительно делает два разных типа вещей. У вас также есть пара массивов, которые вы в зависимости от адресации параллельно. Я бы реструктурировал его на две отдельные функции: roll и re_roll. Я бы реструктурировать данные в виде массива структур:

struct die_roll { 
    int value; 
    bool keep; 

    die_roll() : value(0), keep(true) {} 
}; 

Чтобы сделать первоначальный бросок, вы передаете вектор (или массив если вы действительно настаиваете) из них функции roll, которая заполняет в начальных значениях , Чтобы выполнить повторный ролл, вы передаете вектор в re-roll, который перематывает, чтобы получить новое значение для любого die_roll, чей член keep был установлен в false.

Answer 2

Используйте (Пакетирование выделено) std::vector вместо массива и передать ссылку на функцию. Таким образом, вы убедитесь, что он не течет.

Answer 3

То, как вы распределяете память, сбивает с толку: память, выделенная внутри функции, должна быть освобождена кодом вне функции.

Почему бы не переписать это что-то вроде этого:

int *kast = new int[DICE];   //rolled dice 
bool *keep_dice = new bool[DICE]; //which dice to re-roll or keep 
for (int i = 0; i < DICE; ++i) 
    keep_dice[i] = false; 

roll(kast, keep_dice); 

delete[] kast; 
delete[] keep_dice; 

Это соответствует вашим new с и delete S приятно. Что касается функции: потому что мы установили keep_dice все в false, ни один из аргументов являются либо NULL, и она всегда изменяет dice вместо возвращения нового массива, это упрощает для:

void roll(int *dice, int *keep) { 
    for(int i=0;i<DICE;i++) 
    { 
     if(keep[i]) 
     { 
      keep[i] = false; 
      cout << "Keep "; 
     } 
     else 
     { 
      dice[i] = (rand()%6)+1; 
      cout << "Change "; 
     } 
    } 
    cout << endl; 
} 

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

Answer 4

Мое предложение - взять тайм-аут, чтобы купить/заняться и прочитать Scott Meyers Effective C++ 3rd Edition. Вы спасете себе месяцы боли, чтобы стать продуктивным программистом на C++. И я говорю из личного, горького опыта.