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 and read through it. You should really use std::vector<double> instead of a raw array; I can already see a memory leak in the set_valuesCoord() function. — In silico
– In silico, Commented Dec 14, 2010 at 18:17
There is much wrong with your code. Please refer to the books link @In silico gave you. — John Dibling
– John Dibling, Commented Dec 14, 2010 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
– Drew C, Commented Dec 14, 2010 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
– Sergei Tachenov, Commented Dec 14, 2010 at 19:20

wilhelmtell · Accepted Answer · 2010-12-14 18:26:21Z

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, 2010 at 18:26

wilhelmtell

59k20 gold badges98 silver badges131 bronze badges

Sign up to request clarification or add additional context in comments.

6 Comments

Drew C Over a year ago

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 Over a year ago

Is there anything fundamentally wrong about assigning a dynamic array size in the function?

Drew C Over a year ago

ok, I am a dum dum, sorry for wasting your time realised it should have been: ClusterCoord = new double [Entries];

wilhelmtell Over a year ago

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.

Drew C Over a year ago

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.

|

Mark B · Accepted Answer · 2010-12-14 19:34:40Z

1

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');
    }
}

answered Dec 14, 2010 at 19:34

Mark B

96.5k10 gold badges113 silver badges198 bronze badges

1 Comment

cppcoder Over a year ago

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

Collectives™ on Stack Overflow

Initializing a dynamic memory array in a class

2 Answers 2

6 Comments

1 Comment

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

6 Comments

1 Comment

Your Answer

Sign up or log in

Post as a guest

Linked

Related