C++ will this function leak?

Question

I have started out to write a simple console Yahtzee game for practice. I just have a question regarding whether or not this function will leak memory. The roll function is called every time the dices need to be re-rolled.

What it does is to create a dynamic array. First time it is used it will store 5 random values. For the next run it will only re-roll all except for the dice you want to keep. I have another function for that, but since it isn't relevant for this question I left it out

Main function

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

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

and here's the function

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

Yes, it can leak. Just for example, using cout can throw an exception, and if it does, your delete will never be called.

Instead of allocating a dynamic array yourself, you might want to consider returning an std::vector. Better still, turn your function into a proper algorithm, that takes an iterator (in this case, a back_insert_iterator) and writes its output there.

Edit: Looking at it more carefully, I feel obliged to point out that I really dislike the basic structure of this code completely. You have one function that's really doing two different kinds of things. You also have a pair of arrays that you're depending on addressing in parallel. I'd restructure it into two separate functions, a roll and a re_roll. I'd restructure the data as an array of structs:

struct die_roll { 
    int value;
    bool keep;

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

To do an initial roll, you pass a vector (or array, if you truly insist) of these to the roll function, which fills in initial values. To do a re-roll, you pass the vector to re-roll which re-rolls to get a new value for any die_roll whose keep member has been set to false.

Answer 2

Use a (stack-allocated) std::vector instead of the array, and pass a reference to it to the function. That way, you'll be sure it doesn't leak.

Answer 3

The way you allocate memory is confusing: memory allocated inside the function must be freed by code outside the function.

Why not rewrite it something like this:

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;

This matches your news and deletes up nicely. As to the function: because we set keep_dice all to false, neither argument is ever NULL, and it always modifies dice instead of returning a new array, it simplifies to:

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

Also, you should move the srand call to the start of your program. Re-seeding is extremely bad for randomness.

Answer 4

My suggestion would be to take time out to buy/borrow and read Scott Meyers Effective C++ 3rd Edition. You will save yourselves months of pain in ramping up to become a productive C++ programmer. And I speak from personal, bitter experience.