Adding element to array of struct c++

Question

Can someone explain why this code does not work? It keeps crashing when it asks for input in addCar().

I think something is wrong with copying an array, but I can't figure out what exactly. I also tried to use copy() but it didn't work either.

#include <iostream>
#include <string>
using namespace std;
/* run this program using the console pauser or add your own getch, system("pause") or input loop */


struct Car{    
        string Brand;
        string model;
        long mileage;
};

void addCar(int *ptr, struct Car *arra){
        *ptr=*ptr+1;

        Car *newArr = new Car[*ptr];

        memcpy(newArr, arra, (*ptr)*sizeof(Car));

        cout<<"Brand ";
        getline(cin,newArr[*ptr].Brand);

        cout<<"Model ";
        getline(cin, newArr[*ptr].model);

        cout<<"mileage ";
        cin>>newArr[*ptr].mileage;

        arra=newArr;    
};

int main(int argc, char** argv) {

        int size=1;
        int *ptr_size;
        ptr_size=&size;

        Car *tab=new Car[*ptr_size];

        tab[0].Brand = "Audi";
        tab[0].model = "A8";
        tab[0].mileage = 14366;

        addCar(*ptr_size, tab);

        return 0;    
}

You're passing an integer into the first argument of `addCar`, which wants a pointer. — lost_in_the_source, May 19 '15 at 21:06
`memcpy` will cause downstream problems if used with a non-trivially copyable object. See http://stackoverflow.com/questions/29777492/why-would-the-behavior-of-stdmemcpy-be-undefined-for-objects-that-are-not-triv. — R Sahu, May 19 '15 at 21:11
@ChrisS `ptr_rozmiar` was supposed to be `ptr_size`. I forgot to change it. — marnow, May 19 '15 at 21:39
@marnow `#include std::vector allCars; Car myNewCar; allCars.push_back(myNewCar);` It is as simple as that. — PaulMcKenzie, May 19 '15 at 22:05
That's assuming the assignment didn't ban the use of `std::vector`. If it did, it's worth asking the instructor why. There are quite a few really good ways to teach arrays and pointers without teaching students to use dangerous subsets of a language. — user4581301, May 19 '15 at 22:17

score 1 · Accepted Answer · answered May 19 '15 at 23:40

The fail is probably here:

getline(cin,newArr[*ptr].Brand);

A bit above, you did this: *ptr=*ptr+1; and made newArr an array of *ptr elements. Arrays are origin zero. That means the first item in the array is newArr[0]. The last will be at newArr[*ptr-1], so writing into newArr[*ptr] is writing over someone else's memory. Generally a bad thing to do.

But this is also not cool:

*ptr=*ptr+1;

Car *newArr = new Car[size+1];

memcpy(newArr, arra, (*ptr)*sizeof(Car));

You increment the size of the array. That's OK.

You create a new array with the new size. That's OK.

You copy new size number of elements from the old array to the new array and over shoot the end of the old array. Not OK.

The best answer is given by Jerry Coffin and Paul McKenzie in the comments: use a std::vector. If this is not allowed... Ick.

But alrighty then.

First, memcpy literally copies a block of memory. It does not know or care what that block of memory is or what it contains. Never use memcpy unless you are copying something really, really simple like basic data type or a structure made up of nothing but basic data types. String is not basic. The data represented by a string might not be inside the string. In that case, you copy a pointer to the string and that pointer will not be valid after the death of the string. That's not a problem in your case because you don't kill the string. That leads to problem 2. Let's fix that before you get there. The easiest way (other than vector) is going to be:

for (int index = 0; index < *ptr-1; index++)
{
    newArr[index] = arra[index];
}

An optimization note. You don't want to resize and copy the array every time you add to it. Consider having two integers, one size of array and the other index into array and double the size of the array every time the index is about to catch up with the size.

When you allocate any memory for data with new somebody has to clean up and put that memory back with delete. In C++ that somebody is you. so, before you arra=newArr; you need to delete[] arra;

Passing in the array index as a pointer overcomplicates. Use a reference or just pass by value and return the new index. Also, don't name a variable ptr. Use something descriptive.

void addCar(int &arrasize, struct Car *arra){

or

int addCar(int arrasize, struct Car *arra){

Next problem: int addCar(int arrasize, struct Car *arra){ passes in a pointer to arra. But you passed the pointer by value, made a copy of the pointer, so when you change the pointer inside the function, it's only the copy that got changed and the new array is not going to come back out again. So,

int addCar(int arrasize, struct Car * & arra){

Passes in a reference to the pointer and allows you to modify the pointer inside the function.

Putting all that together:

int addCar(int size, struct Car * & arra)
{
    Car *newArr = new Car[size + 1];

    for (int index = 0; index < size; index++)
    {
        newArr[index] = arra[index];
    }

    cout << "Brand ";
    getline(cin, newArr[size].Brand);

    cout << "Model ";
    getline(cin, newArr[size].model);

    cout << "mileage ";
    cin >> newArr[size].mileage;

    delete[] arra;
    arra = newArr;
    return size+1;
}

int main()
{
    int size=1;

     Car *tab=new Car[size];

     tab[0].Brand = "Audi";
     tab[0].model = "A8";
     tab[0].mileage = 14366;

     size = addCar(size, tab);

     // do more stuff;
     // bit of test code here
     for (int index = 0; index < size; index++)
     {
         cout << "Car " << index << " brand =" <<tab[index].Brand << " Model=" << tab[index].model << " mileage=" <<tab[index].mileage << endl;
     }
     delete[] tab;
     return 0;
}

score 0 · Answer 2 · answered May 19 '15 at 23:35

When you are copying the old array to the new one you are accessing invalid memory, remember that, in that point, arra has size *ptr-1 not *ptr, so the line should be

        memcpy(newArr, arra, (*ptr-1)*sizeof(Car));

also in the other lines you should insert the new value in the *ptr-1 position because the indexes in newArr go from 0 to size-1 ie *ptr-1:

    cout<<"Brand ";
    getline(cin,newArr[*ptr-1].Brand);

    cout<<"Model ";
    getline(cin, newArr[*ptr-1].model);

    cout<<"mileage ";
    cin>>newArr[*ptr-1].mileage;

Adding element to array of struct c++

2 Answers2