Initializing a dynamic memory array in a class

Question

I am new to c++ programming, and this is probably a trivial problem, but I need to construct a variable sized array in a class and transfer text file data into it, see below. Here HISTORYFile >> ClusterCoord[i]; seems to take in the information fine, however when I try to get access to the info in the main program via,

cout << CoordClassExample.ClusterCoord[1] << "\n";

I get a bus error. Please help if you can!

class CoordClass{
public:
    int Entries;
    double * ClusterCoord;
    void set_valuesCoord(ifstream &HISTORYFile,int MolAtomNum, int MolNum);
};

void CoordClass::set_valuesCoord(ifstream& HISTORYFile,int MolAtomNum, int MolNum) {
    Entries=MolAtomNum*MolNum;
    double *ClusterCoord = new double [Entries];

    for (int i=0;i<Entries;i++) {
        HISTORYFile.ignore(1000,'\n');      
            HISTORYFile >> ClusterCoord[i];
        cout << ClusterCoord[i] << "\n";
            HISTORYFile.ignore(1000,'\n');
    }
}

If you're new to C++, please [pick up a good C++ book](http://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list) and read through it. You should really use `std::vector` instead of a raw array; I can already see a memory leak in the `set_valuesCoord()` function. — In silico, Dec 14 '10 at 18:17
There is much wrong with your code. Please refer to the books link @In silico gave you. — John Dibling, Dec 14 '10 at 18:19
Yeah, perhaps I should, just trying to recreate a program I wrote in fortran on the fly. Even if you could just give me what to search for that would be helpful. And perhaps highlight the memory leak line. — Drew C, Dec 14 '10 at 18:24
Remember that for every `new` there should be a corresponding `delete` (`delete[]` for arrays). You allocate memory once, you free it once. Otherwise you'd either get a memory leak (allocate and not free) or something more terrible (free without allocating or allocate and free more than once). In your case, there is no `delete[]` anywhere. — Sergei Tachenov, Dec 14 '10 at 19:20

score 1 · Answer 1 · answered Dec 14 '10 at 18:26

1

You have a leak in the set_valuesCoord() function if you call the function twice, unless you somewhere release the resources. That's not the problem but it's a problem. Use a std::vector<>.

class CoordClass {
    // ...
    std::vector<double> ClusterCoord;  // instead of double *ClusterCoord
    // ...
};

What might be the problem is that you don't check whether the double parsed properly. If it didn't then you're accessing uninitialized memory, and that leads to undefined behaviour.

void CoordClass::set_valuesCoord(...)
{
    // ...
    double cluster_coord = 0;
    if( HISTORYFile >> cluster_coord )
        ClusterCoord.push_back(cluster_coord);
    else
        std::cerr << "Error parsing cluster coord.\n";
    // ...
}

answered Dec 14 '10 at 18:26

wilhelmtell

57,473
20
96
131

Thanks for the tip. The double definately did parse properly, I have run this just fixing the size of the array in advance. – Drew C Dec 14 '10 at 18:39
Is there anything fundamentally wrong about assigning a dynamic array size in the function? – Drew C Dec 14 '10 at 18:41
ok, I am a dum dum, sorry for wasting your time realised it should have been: ClusterCoord = new double [Entries]; – Drew C Dec 14 '10 at 18:43
There's nothing fundamentally wrong with using primitive pointers and dynamic allocation for dynamic arrays. The problem with that is that you need to manage the memory yourself, and that leaves room for mistakes. I can see from your code that indeed you're doing it poorly. Sometimes it actually is hard to do it right: what happens if there's an exception thrown? Unless you thought about it then the memory you allocated in the none-catching scope will leak. – wilhelmtell Dec 14 '10 at 18:57
Thanks again for all your help. In the past I was programming fortran, where memory management is a lot more straightforward. This will be a learning curve to get this right. – Drew C Dec 14 '10 at 19:08
@Drew C if you use C++ features like containers (`vector` in this case) and `shared_ptr` you won't have to worry about managing memory manually. – Mark B Dec 14 '10 at 19:28

score 1 · Answer 2 · answered Dec 14 '10 at 19:34

As an exercise showing the vector way that won't leak among other things:

Further changes would be to remove Entries and use ClusterCoord.size().

class CoordClass{
public:
    int Entries;
    std::vector<double> ClusterCoord;
    void set_valuesCoord(ifstream &HISTORYFile,int MolAtomNum, int MolNum);
};

void CoordClass::set_valuesCoord(ifstream& HISTORYFile,int MolAtomNum, int MolNum) {
    Entries=MolAtomNum*MolNum;
    ClusterCoord.resize(Entries);

    for (int i=0;i<Entries;i++) {
        HISTORYFile.ignore(1000,'\n');      
            HISTORYFile >> ClusterCoord[i];
        cout << ClusterCoord[i] << "\n";
            HISTORYFile.ignore(1000,'\n');
    }
}

this is a better answer than the previous one as it resizes the vector in advance (+1 for that). So, the penalty for memory allocation and copying incurred in previous example is avoided. Apart from memory leak, if ClusterCoord is not deleted in the destructor of the class, I do not see the reason for a bus error. Surprisingly I did not see a reply for this..Any takers. thanks — cppcoder, Sep 05 '11 at 23:31

Initializing a dynamic memory array in a class

2 Answers2