CODEWITHSUNDEEP
c++visual-c++
laura
laura

Reputation: 2135

Will this code cause memory leak issue?

Considering the code below, what happens if i use an instance of Die class like this:

Die d;
d.Roll(20);
d.Roll(15);
d.Roll(30);

Should I or should I not free the memory occupied by values before alocating again memory for it? delete[ ] before new?

die.h

#ifndef DIE_H
#define DIE_H
#include<iostream>
#include<time.h>
using namespace std;


class Die
{
private:
    int number;
    int* values;
    int count;
    void roll();
public:
    Die(){srand(static_cast<int>(time(NULL)));number=0;values=NULL;count=0;}
    void Roll(int n);
    int getNumber()const{return number;}
    void printLastValue();
    void printValues();
    ~Die(){delete [] values;}

};

#endif

die.cpp

#include"die.h"
#include<iostream>
#include<time.h>
using namespace std;

void Die::roll()
{

    number=1+rand()%6;
}

void Die::printLastValue()
{
    cout<<number<<endl;
}

void Die::Roll(int n)
{
    count=n;
    values=new int[count];
    for(int i=0;i<count;i++)
    {
        roll();
        values[i]=number;
    }

}
void Die::printValues()
{
    for(int i=0;i<count;i++)
    {
        cout<<values[i]<<endl;
    }
}

main.cpp

#include"die.h"
#include<iostream>
using namespace std;

int main()
{
    Die d;
    d.Roll(25);
    d.printValues();
    d.Roll(40);
    d.printValues();
    d.Roll(100);
    d.printValues();
    d.printLastValue();
}

Upvotes: 2

Views: 356

Answers (3)

pstrjds
pstrjds

Reputation: 17448

Yes, this will cause a memory leak if you call Roll multiple times. You should check if values is NULL and call delete [] if it is not.

EDIT:
As commented below, you do not have to check for null, you can safely call delete on a null pointer. Its just a long engrained habit from company standards where I use to work.

You should look into using a std::vector instead of the array. By doing this you will remove the danger of a memory leak and you will no longer need to explicitly define a destructor. You could replace your the values with this:

std::vector<int> values;

Then in your Roll code you could do this:

void Die::Roll(int n) {
    count=n;
    values.clear();
    for(int i=0;i<count;i++)
    {
        roll();
        values.push_back(number);
    }
}

Upvotes: 5

Rajat
Rajat

Reputation: 1836

Yes, it will leak memory. When you do

Values = new int [len];

It allocates new memory with the array and points values to the new memory location. The old memory location still contains the old data which needs to be deleted before the new data can be allocated.

Upvotes: 1

user1775315
user1775315

Reputation:

You definitely need to delete them because you are reassigning Die::values, causing memory leaks.

EDIT: in this case it's better to use std::vector than a raw array. Then you don't need to delete anything, but simply call std::vector::clear at the beginning of Die::Roll.

Upvotes: 3

Related Questions