C++ Trouble deleting dynamic sized array of pairs

Question

I am having lots of problems deleting m_Array. The program segfaults at the end when it is doing the clean-up portion. I have two class A objects with different data in m_Array, and at some point in the program the data from one object starts to "wrap around" into the other array, causing incorrect data. T represents my templated data type.

There is also a class B, which just creates two class A objects

Declared publicly in class declaration A like:

template <typename T> class A
{
public:
    pair<T, int> *m_Array;  // Array of type pair
    A(int size=1);       // constructor
    ~A();     // A destructor

    // ... all other definitions //

 };

Defined in class A's constructor definition like:

template <typename T>
A<T>::A(int size) {

    // Set array size
    m_Array = new pair<T, int>[size];

    // Initialize values
    for (int i = 0; i < size; i++) {
        m_Array[i] = make_pair(-1, -1);
    }

    //... other things defined and initialized...//
}

In class A's destructor:

template <typename T>
A<T>::~A() {

        delete [] m_Array; // Not working as it should
}

Overloaded assignment operator

template <typename T>
const A<T>& A<T>::operator=(const A<T>& rhs) {
    m_AArraySize = rhs.m_AArraySize;
    m_currASize = rhs.m_currASize;

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

        m_Array[i].first = rhs.m_Array[i].first;
        m_Array[i].second = rhs.m_Array[i].second;
    }

    _ptr = rhs._ptr;

    return *this;
}

Copy constructor

template <typename T>
A<T>::A(const A<T>& other) {
    m_AArraySize = other.m_AArraySize;
    m_AHeapSize = other.m_AHeapSize;

    for (int i = 0; i < m_currASize; i++) {
        m_Array[i].first = other.m_Array[i].first;
        m_Array[i].second = other.m_Array[i].second;
    }

    _ptr = other._ptr;
}

Class B declaration

template <typename T> class B{
public:
    //B constructor
    B(int size);


    int m_currBSize;        // spots used in array
    int m_BSize;            // size of array

    A <T> oneAHolder;
    A <T> twoAHolder;

};

Class B constructor

template <typename T>
b<T>::b(int size){
    A<T>(size);

    m_BArraySize = size;
    m_currBSize = 1;

    // Create two A objects
    A<T> oneA(size);
    A<T> twoA(size);

    // oneA and twoA go out of scope
    oneAHolder = oneA;
    twoAHolder = twoA;
}

In my main function all that is being done is I am creating a class B object, and using it's insert function to insert the data into both of its A objects.

I have tried several different ways of deleting the data from the array, and stopping the data overflowing into the other array, but to no avail.

I appreciate any help!

P.S.: Please no "Just use std::vector"

EDIT: Added more of my code

What do you mean by "wrap around"? It sounds more like a double free problem to me. Are you using copy ctor or assigning one object A to another `A newA = anotherA`? Take a look at this https://stackoverflow.com/questions/7823845/disable-compiler-generated-copy-assignment-operator or implement those functions. — Tony J, Apr 23 '17 at 02:13
Show us your `main` program. With what you posted, the program can be easily broken with two lines of code. — PaulMcKenzie, Apr 23 '17 at 02:21
By the way, it seem you forgot to implement move / copy constructor. Is this intended? — Guillaume Racicot, Apr 23 '17 at 03:03
There is a copy constructor and overloaded assignment operator for class A in the code above. — Grehgous, Apr 23 '17 at 03:04
When you post code, don't try to "break it into bite sized pieces", just post the code you're compiling as described here: [mcve] What you did makes it harder for people to help you and it's not known what piece of important information you may have left out because you didn't realize it was important. — xaxxon, Apr 23 '17 at 03:16
@Grehgous The assignment operator leaks memory. It's supposed to create new memory and assign to the new memory the contents of the passed-in class. Then it's supposed to delete the old memory. — PaulMcKenzie, Apr 23 '17 at 13:30

score 1 · Accepted Answer · edited May 23 '17 at 12:17

Given the code you posted, your assignment operator has two issues:

1) The assignment operator leaks memory, since you failed to deallocate the old memory.

2) Unless this->m_Array was already large enough, you are overwriting memory in the for loop since this->m_Array is a smaller buffer than rhs.m_Array.

Instead of all of this faulty code, you could have simply used the copy/swap idiom.

#include <algorithm>
//...
template <typename T>
A<T>& A<T>::operator=(const A<T>& rhs) 
{
    A<T> temp(rhs);
    std::swap(temp.m_AArraySize, m_AArraySize);
    std::swap(temp.m_currASize, m_currASize);
    std::swap(temp.m_Array, m_Array);
    std::swap(temp._ptr, _ptr);
    return *this;
}

This works if you have written a correct copy constructor that does not indirectly call the assignment operator, and correct destructor. If any of these functions are faulty, the above method will not work.

Assuming that the copy constructor and destructor have no bugs, writing the assignment operator this way avoids rewriting code from the copy constructor since you're merely just reusing it.

If there are more member variables in A that you did not specify in your post, they all need to swapped also.

In short, this basically just makes a copy of rhs and exchanges all of the old data from this with temp. Then temp dies off at the end with the old data.

C++ Trouble deleting dynamic sized array of pairs

1 Answers1