How to handle passing runtime-sized arrays between classes in C++

Question

Right now I have a simple class that handles the parsing of XML files into ints that are useful to me. Looks something like this:

int* DataParser::getInts(){
    *objectNumbers = new int[getSize()];
    for (int i=0;i<getSize();i++){
            objectNumbers[i]=activeNode->GetNextChild()->GetContent();
    }
    return objectNumbers;
 }

In the main part of the program, I receive this by doing:

int* numbers= data->getInts();
///Do things to numbers[]
delete numbers;

Everything works fine until the delete command, which crashes everything. What is the proper way of doing this?

Answer 1

The following line:

 *objectNumbers = new int[getSize()];

What does it do? If you are returning objectNumbers, this is a pointer to int, and you should really be doing:

 objectNumbers = new int[getSize()];

Anyway, C++ gives you collections (vector, list etc) -- I'd have used one of those instead of a plain array. As noted elsewhere it is important to match your new with a delete and a new [] with a delete [].

Passing arrays around isn't good design -- you are making the implementation public. Try to pass iterators to the begining and end of the array/collection/sequence of ints instead following the STL design.

Answer 2

You need to

delete [] numbers;

The rules is whenever you

ptr = new Type[...];

make sure you

delete [] ptr;

instead of the regular

delete ptr;

which will result in undefined behavior (thanks Neil Butterworth) and is intended for deletion of a single instance where ptr points, not an array.

Answer 3

Part of the problem is that you are not pairing new[] with delete[]. This probably isn't the root of your bug here but you should get in the habbit of doing this.

The bug is almost certainly related to the code that you left commented out. Can you add some more context there so we can see what you're doing with the numbers value?

In general, I find it's much easier to use a vector for this type of problem. It takes the memory management out of the equation and has the added benefit of storing the size with the dynamic memory.

void DataParser::getInts(std::vector<int>& objectNumbers){
    for (int i=0;i<getSize();i++){
      objectNumbers.push_back(activeNode->GetNextChild()->GetContent());
    }
 }

...
std::vector<int> numbers;
data.getInts(numbers);

Answer 4

You will quickly run into trouble and maintenance issues. Consider using std::vector, this is the proper way to do it.

Answer 5

simply use a std::vector instead;

std::vector<int> DataParser::getInts(){
    std::vector<int> objectNumbers(getSize());
    for (int i=0;i<getSize();i++){
            objectNumbers[i]=activeNode->GetNextChild()->GetContent();
    }
    return objectNumbers;
 }