Are there any memory leaks here??

Code:

---

bool AddRecord(StudentRecord**& Records, int& numRecords){
/*Please tell me if I have memory leaks.*/       
/*       
        struct StudentRecord
{
        char* firstName;                        // student key
        char* lastName;                                // student name
        int id;                                                // student id
        float mark;                                        // course mark
};*/
       
        char FirstName[MaxRecord];char LastName[MaxRecord];int id; float mark;

       
       
StudentRecord**Temp; /*Declare array of pointers to a struct*/
        Temp=new StudentRecord*[numRecords];
       

        cout<<"Please enter First Name:\n";
        cin>>FirstName;
        cout<<"Please enter Last Name:\n";\
        cin>>LastName;
        cout<<"Please enter ID:\n";
        cin>>id;
        cout<< "Please enter mark\n";
                cin>>mark;
        numRecords++;
       
        for (int i=0; i<numRecords;i++)
        {
        /*dynamically allocate memory*/
        Temp[i] = new StudentRecord; // allocate space for student record
        Temp[i]->firstName=new char[strlen(FirstName)+1];
        Temp[i]->lastName=new char[strlen(LastName)+1];
        strcpy(Temp[i]->firstName,FirstName);
        strcpy(Temp[i]->lastName,LastName);
        Temp[i]->id=id;
        Temp[i]->mark=mark;
        }
        if (numRecords==1)
        {
                Records=new StudentRecord*[numRecords];
                for        (int j=0;j<numRecords;j++)
                {
                       
                        Records[j] = new StudentRecord;
                        Records[j]->firstName=new char[strlen(Temp[j]->firstName)+1];
                        Records[j]->lastName=new char[strlen(Temp[j]->lastName)+1];
                        strcpy(Records[j]->firstName,Temp[j]->firstName);
                        strcpy(Records[j]->lastName,Temp[j]->lastName);
                        Records[j]->id=Temp[j]->id;
                        Records[j]->mark=Temp[j]->mark;
                }
        }
        else if (numRecords>1)
        {
                for (int k=numRecords-1, l=0; l<numRecords, k<numRecords;k++,l++)
                        /*get ready to write from last records in studentrecord** Records by copying
                        studentrecord **temp starting at 0*/
                {
                        Records[k] = new StudentRecord;// allocate space for student record
                        Records[k]->firstName=new char[strlen(Temp[l]->firstName)+1];
                        Records[k]->lastName=new char[strlen(Temp[l]->lastName)+1];
                        strcpy(Records[k]->firstName,Temp[l]->firstName);
                        strcpy(Records[k]->lastName,Temp[l]->lastName);
                        Records[k]->id=Temp[l]->id;
                        Records[k]->mark=Temp[l]->mark;
                }
        }
       


        //delete []Temp; /*crashes when I use this*/
        return true;
}

---

>> Temp=new StudentRecord*[numRecords];
When that line executes, what is the value of numRecords?

You don't really have a memory leak (at least, that's probably not the most pressing problem), but you do have many issues you have with pointers and memory allocation in the posted code. In C++, you don't have to do all that work yourself (and risk having so many errors that could lead to crashes). You can use the standard C++ vector and string classes.

If you use a vector instead of your dynamic array, then you can use push_back to add new records to the vector instead of having to identify how many records will be created from the start. There are a bunch of other benefits as well. Is there a reason you aren't using the C++ features available?

Quote:

---

Originally Posted by Daved

>> Temp=new StudentRecord*[numRecords];
When that line executes, what is the value of numRecords?

You don't really have a memory leak (at least, that's probably not the most pressing problem), but you do have many issues you have with pointers and memory allocation in the posted code. In C++, you don't have to do all that work yourself (and risk having so many errors that could lead to crashes). You can use the standard C++ vector and string classes.

If you use a vector instead of your dynamic array, then you can use push_back to add new records to the vector instead of having to identify how many records will be created from the start. There are a bunch of other benefits as well. Is there a reason you aren't using the C++ features available?

---

it is an assignment requirement to do it this way. It's the prof's specification. What would it look like in vectors anyway?

Quote:

---

Originally Posted by cmasri

it is an assignment requirement to do it this way. It's the prof's specification. What would it look like in vectors anyway?

---

the value of num records is 0 when that line executes.

>> the value of num records is 0 when that line executes.
Exactly. You are allocating space for an array and specifying 0 as the number of objects you want in that array.

Since this is an AddRecord function, maybe you meant to increase the value of numRecords first, and then allocate space for the temp array? So if you add a single record and numRecords is 0, then you would increment numRecords first to get to 1, then allocate space for it. Does that make sense?

>> for (int i=0; i<numRecords;i++)
What is this for loop supposed to do? Is it copying the old data, or just inserting the new record?

the loop that you are referring to is copying old data from a temporary array.

made improvements according to your suggestion, is that better?

Code:

---

StudentRecord**Temp; /*Declare array of pointers to a struct*/
       
        cout<<"Please enter First Name:\n";
        cin>>FirstName;
        cout<<"Please enter Last Name:\n";\
        cin>>LastName;
        cout<<"Please enter ID:\n";
        cin>>id;
        cout<< "Please enter mark\n";
                cin>>mark;
        numRecords++;
        Temp=new StudentRecord*[numRecords];

---

>> is that better?
Yes, I think that should fix that particular problem.

>> the loop that you are referring to is copying old data from a temporary array.
I don't see where you access the old data. It looks like you are copying the data input by the user over and over again into each slot in the temporary array, so that when the loop is done your temp array will be full of the same data in each spot. I think you need to change it to copy over the data from the old array itself.

Quote:

---

Originally Posted by Daved

>> is that better?
Yes, I think that should fix that particular problem.

>> the loop that you are referring to is copying old data from a temporary array.
I don't see where you access the old data. It looks like you are copying the data input by the user over and over again into each slot in the temporary array, so that when the loop is done your temp array will be full of the same data in each spot. I think you need to change it to copy over the data from the old array itself.

---

thank you that works.
another question....

Code:

---

void ListFileMem(StudentRecord** Records, int numRecords){
        cout<<"LIST RECORDS FROM MEMORY";
        string fullname, firstname, lastname,space(" ");
        if (numRecords>0)
        {
                for (int k=0; k<numRecords;k++)
                {
                        firstname=Records[k]->firstName;
                        firstname+=" ";
                        lastname=Records[k]->lastName;
                        fullname=firstname+lastname; /*compiler says i can't do this, why??*/
                        cout<<"Record"<<k<<"\n"<< "First Name: "<<fullname<<"\t"<<
                                "Last Name: "<< Records[k]->lastName<<"\t"<<"ID:"<<Records[k]->id
                                <<"\t"<<"Mark:"<<Records[k]->mark<<endl;;
                }
      }
        else
       
                cout<<"memory empty, unable to list\n";




}

---

Code:

---

error from compiler

n:\assignment0.cpp(265): error C2784: 'std::reverse_iterator<_RanIt> std::operator +(_Diff,const std::reverse_iterator<_RanIt> &)' : could not deduce template argument for 'const std::reverse_iterator<_RanIt> &' from 'std::string'

---

the name comes in by 2 char* char *firstname and char*lastname. I want to have the name contatenated when I display it.

example display

Code:

---

Name: Lester Pearson

---

Quote:

---

Originally Posted by cmasri

it is an assignment requirement to do it this way. It's the prof's specification. What would it look like in vectors anyway?

---

indeed, this is not the modern way to code in C++ and if I ever got to work with you and you'd write code like this I would beat you with a rubber dildo. But of course if you're learning about pointers and dynamic memory it's a good way.

In modern C++ it would look more like:

Code:

---

// even better a full fledged class
struct StudentRecord
{
        std::string firstName;                       
        std::string lastName;                       
        int id;                                               
        float mark;                               
};

int main()
{
  std::vector<StudentRecord> students;

  for(...)
  {
    StudentRecord student;
    // read a new student or even overload operator>> for StudentRecord
    students.push_back(student);
  }
 
  return 0;
}

---

you dont see any pointers, new's and deletes in this code anymore