0

so i'm working on an assignment for my c++ class and I am attempting to do different functions using my class with arrays. It works mostly until the overload operator where the memory leak seems to happen.

#include <iostream>
#include <algorithm>  // for std::copy_n
using namespace std;


class MyArray{
private:
double* a{};
int n{};
public:

MyArray(){
    a = nullptr;
    n = 0;
}   // default constructor
/////////////////////////////////////////////////
explicit MyArray(int size){
    n = size;
    a = new double[size]{};

}  //constructor specifying size
/////////////////////////////////////////////////
MyArray(int size, double def_val) {

    n = size;
    a = new double[size];
    for (int i = 0; i < size; ++i) {
         //def_val=i;
        a[i] = def_val;
    }
} //constructor set to default values and specifying size
/////////////////////////////////////////////////
double  GetA(int i){
    return a[i];
}     //setter
/////////////////////////////////////////////////
int GetN() const{
    return n;
}      //getter
/////////////////////////////////////////////////
void  SetN(int x) {
    n = x;
}     //setter
/////////////////////////////////////////////////
void  SetA(int i, double x) {
    a[i] = x;
}     //getter
/////////////////////////////////////////////////
MyArray(const MyArray & a2, int n) {

    a = new double[n]{};
    n = a2.n;
    for (int i=0; i< n; i++){
        a[i]=a2.a[i];
    }
}     //constructor by copy
/////////////////////////////////////////////////
~MyArray(){
    delete []a;
        } // destructor
/////////////////////////////////////////////////
 MyArray operator=(const MyArray& a2) {

        int new_n = a2.n;
        auto *new_data = new double[new_n];
        std::copy_n(a2.a, new_n, new_data);

        delete[] a;

        n = new_n;
        a = new_data;

        return *this;

}  //assignment
int  get_Size() const{
    return n;
}          //accessor
/////////////////////////////////////////////////
void  push_begin(double first) {
    auto* temp = new double [n+1];
    n++;
    for (int i = 0; i < n-1; i++){
        temp[i+1] = a[i];
    }
    temp[0] = first;
    this-> a = temp;
}
/////////////////////////////////////////////////
void  push_end(double last) {
    a[n] = last;
    n++;
} 
/////////////////////////////////////////////////
void  remove_begin() {
    for (int i = 0; i < n - 1; i++) {
        a[i] = a[i + 1];
    }
    n--;
} //done
/////////////////////////////////////////////////
void  remove_End() {
n--;
} 
/////////////////////////////////////////////////
void  reverse() {
    double temp;
    for(int i=0; i<n/2; i++){
        temp = a[i];
        a[i] = a[n-i-1];
        a[n-i-1] = temp;
    }
} 
/////////////////////////////////////////////////
void  display(){
    for (int i = 0; i < n; i++) {
        cout << a[i] << " ";
    }
    cout << endl;
} 

friend MyArray operator + (MyArray &arr1, MyArray &arr2){
    int n1 = arr1.get_Size();
    int m = arr2.get_Size();
    int size = m + n1;
    MyArray ans(arr2, size);
    for(int i=0; i<size; i++){
        ans.push_end(arr2.a[i]);
    }
    return ans;
} 
};

main:

int main(){
MyArray a(9);
MyArray b(10, 5);
b.push_begin(45);
b.push_end(64);
b.push_end(31);
b.push_begin(908);
b.display();
b.display();
b.display();
b.reverse();
b.display();
MyArray c (a+b);
c.display();         //////The leak seems to happen here but I can't find a memory 
                     /////leak finder that works for me
}

I have tried to use valgrind but it doesn't want to run on my laptop.

908 45 8 8 8 8 8 8 64 31
908 45 8 8 8 8 8 8 64 31
908 45 8 8 8 8 8 8 64 31
31 64 8 8 8 8 8 8 45 908
The Max value of the concatenated array is: 908
31 64 8 8 8 8 8 8 45 908 6.93003e-310 6.93003e-310 0 0 0

This is what is output and I have no possible clue where 6.93003e-310 comes from when it should most definitely be 0.

submartingale
  • 715
  • 7
  • 16
  • `MyArray operator=(const MyArray& a2)` -- You're supposed to return a reference to the current object, not a brand new object. – PaulMcKenzie Sep 16 '21 at 22:55
  • `MyArray ans(arr2, size);` -- `for(int i=0; i – PaulMcKenzie Sep 16 '21 at 23:00
  • And this brings us back to [Copy and Swap](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom) from the earlier question. Note that Copy and Swap isn't always the best solution, but it's quick to write and next to impossible to get wrong. That combination makes it the perfect starting point. If it turns out to be too slow for your application, change it later. – user4581301 Sep 16 '21 at 23:07
  • Valgrind is pretty much a *nux-only tool. It can be worth having a Linux virtual machine (or Linux Subsystem for Windows) on your development machine so you can wash tricky programs through it. Also [look into ASan](https://en.wikipedia.org/wiki/AddressSanitizer) and associated tools. I think these are supported on Windows with the newer releases of clang. – user4581301 Sep 16 '21 at 23:10
  • Also, `push_end` doesn't do that. I would expect it to expand the array, i.e. make an array of size x equal to size x+1, and add an item to the back. – PaulMcKenzie Sep 16 '21 at 23:12
  • Another point -- your `main` program fails to explicitly exercise `operator =`. Somewhere, there should be a line or several lines where you are doing something like `b = c;` and check if `b` does actually contain the same elements as `c`, and you have no memory-related errors. – PaulMcKenzie Sep 16 '21 at 23:22

1 Answers1

1

The problem is that your class has no copy constructor. MyArray(const MyArray & a2, int n) is not a copy constructor because it requires an additional argument, n. Which you don’t actually need as you have a2.n.

Also, you leak a in push_begin, and forget to resize it at all in push_end (as well as in setN but you don’t seem to use it at all).

Finally, your operator+ is indeed flawed in several ways. It could be written in one of several ways:

  1. Create an empty new array, then push_back elements from arr1, then push_back elements from arr2.
  2. Create a copy of arr1, then push_back elements from arr2.
  3. Create an array of size arr1.n + arr2.n and fill it directly, without push_back.

What is in the code is a weird mix of these 3 approaches.

And a stylistic note: operator= should return a reference (as pointed out in comments), and operator+ should accept const references.

Still, this is better than most homeworks I’ve seen here on SO.

numzero
  • 2,009
  • 6
  • 6
  • 1
    Note that a copy constructor can have default arguments after the first one so long as the single argument copy constructor can be invoked. But you're right in that in the current code, that is not a true copy constructor. – PaulMcKenzie Sep 16 '21 at 23:14