Initializing multiple structs in cpp function

Question

I want to initialize structs in loop and update to array. Following code I am using.

#include <iostream>

struct Record {
    char *name;
};

struct Response {
    Record *records[];
};


int main()
{
    Response *response = new Response;

    for(int i=0; i<4; i++) {

        Record *record= new Record();

        char x[20];sprintf(x, "%d", i);

        record->name = (char*)x;

        response->records[i] = record;

        std::cout << "Inserting: " <<  x << "\n";

        //Not sure if I have to delete.
        delete record;
    }

    for(int i=0; i<4; i++) {
        std::cout << "Fetching: " << response->records[i]->name << "\n";
    }
}

Strangely while printing all items in array are printing same value. Any idea, comment or thoughts on what could be wrong here ?

Sample output:

Inserting: 0
Inserting: 1
Inserting: 2
Inserting: 3
Fetching: 3
Fetching: 3
Fetching:
Fetching: 3

You don't really need to do all that manual memory allocation/de-allocation in C++. Your code doesn't take any advantage of the C+ language or standard library. — juanchopanza, Feb 13 '15 at 15:39
If you didn't use any pointers, your problems would magically disappear. — chris, Feb 13 '15 at 15:39
The array `records` in the `Response` structure is unsized, which means that any index in it will be out of bounds. — Some programmer dude, Feb 13 '15 at 15:40
You should get a [good book](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) and learn how to write *actual* C++-code instead of what ever this is. — Baum mit Augen, Feb 13 '15 at 15:40
Use `std::string` and `std::vector`. There should be NO `new` keywords in your final program. — Neil Kirk, Feb 13 '15 at 15:42
Thank you all for your feedback. I am beginner in C/C++ and definitely I will read good books. — Royal Pinto, Feb 14 '15 at 05:21

score 6 · Accepted Answer · answered Feb 13 '15 at 15:49

You're storing pointers to local variables in objects which you access after having deleted them.

Even if Response::records were properly sized, these two factors would make the code invalid.

Idiomatic C++ would use std::string and std::vector<Record>, but if you really want C-style strings and arrays, this should work:

struct Record {
    char name[20];
};

struct Response {
    Record records[4];
};

int main()
{
   Response response;

    for(int i = 0; i < 4; i++) {
       Record record;
       sprintf(record.name, "%d", i);
       response.records[i] = record;
       std::cout << "Inserting: " <<  record.name << "\n";
    }

    for(int i = 0; i < 4; i++) {
       std::cout << "Fetching: " << response.records[i].name << "\n";
    }
}

tipaye · Answer 2 · 2015-02-13T16:24:18.747

The biggest problem here is that this is C not C++, however, let's consider a C solution:

There are a few problems:

struct Response {
    Record *records[];
};

Don't forget to give a dimension for your array:

Record *records[4];

struct Record {
    char *name;
};

This just declares a pointer called name, there is no memory allocated for name to point at. You will need to allocate memory for name in the future, or make it a static array here instead of a pointer.

record->name = (char*)x;

Now name is pointing at a static array x. This will happen every time through the loop, so all the record->name instances will be pointing at the same array. That's why they're all printing the same value :-) You need to copy the string in x to record->name, which you can't do, because there is no storage associated with record->name.

The 2 ways out are:

struct Record {
    char name[20]; // static array
};
...
char x[20] = {0}; // initialise so we're sure of a terminating null
strcpy(record->name, x);

Or

struct Record {
    char *name;
};
...
record->name = new char[20];
strcpy(record->name, x);

Finally,

//Not sure if I have to delete.
delete record;

No, you mustn't delete here, as those records will be used afterwards. If you must delete, set up something like your initialisation loop, but just before the end of main(), only this time you'll be destroying. Of course, srictly speaking, since this is C, you shouldn't be using new and delete. malloc() and free() family of calls are better suited to C style. The main advantage of new and delete over malloc() and free(), is that call the constructors and destructors of allocated objects at the right time, but you haven't used either of those features.

This would be a lot easier, shorter and safer in C++. Must you do it in C or would you consider C++?

#include <iostream>
#include <cstring>

struct Record {
    char *name;
};

struct Response {
    Record *records[4];
};


int main()
{
    Response *response = new Response;

    for(int i=0; i<4; i++) {

        Record *record= new Record();

        char x[20];sprintf(x, "%d", i);

        record->name = new char[20];
        strcpy(record->name, x);

        response->records[i] = record;

        std::cout << "Inserting: " <<  x << "\n";

        //Not sure if I have to delete.
        //delete record;
    }

    for(int i=0; i<4; i++) {
        std::cout << "Fetching: " << response->records[i]->name << "\n";
    }


    // the program is about to exit and the resources will be freed by the system, so this is not strictly necessary
    for(int i=0; i<4; i++) {
        delete [] response->records[i]->name;
        delete response->records[i];
    }

    delete response;
}

EDIT: Here's one possible solution that's more C++ friendly, no raw pointers, all memory allocation is handled by the standard library in string and vector.

#include <iostream>
#include <string>
#include <vector>

using namespace std;

struct Record {
    Record(string record_name) : name (record_name) {}
    string name;
};

struct Response { 
    vector<Record> records;
};

int main()
{
    Response response;
    for(int i=0; i<4; i++) {
        response.records.push_back(Record(to_string(i)));
        std::cout << "Inserting: " << i << "\n";
    }

    for (auto r : response.records) {
        cout << "Fetching: " << r.name << "\n";
    }

}

`record_name` should be passed by const reference. You should use `[const] auto&` or you are making many copies. — Neil Kirk, Feb 13 '15 at 16:29

Initializing multiple structs in cpp function

2 Answers2