0

I have Memory Leak in my code and not sure how to diagnose it. There are 3 files and the main.cpp is the test file therefore cannot be altered. The program is using a Mem Checker and it displays that ==159804== Memcheck, a memory error detector, definitely lost: 280 bytes in 2 blocks.

Main.cpp (Cannot be altered)

#include<iostream>
#include<cstring>
#include"Basket.h"
#include"Basket.h" //intentional

using namespace std;
using namespace sdds;

void printHeader(const char* title)
{
    char oldFill = cout.fill('-');
    cout.width(40);
    cout << "" << endl;

    cout << "|> " << title << endl;

    cout.fill('-');
    cout.width(40);
    cout << "" << endl;
    cout.fill(oldFill);
}

int main()
{
    sdds::Fruit fruits[]{
        {"apple",  0.65},
        {"banana", 1.25},
        {"pear",   0.50},
        {"mango",  0.75},
        {"plum",   2.00},
    };

    {
        printHeader("T1: Default Constructor");

        Basket aBasket;
        cout << aBasket;

        // conversion to bool operator
        if (aBasket)
            cout << "Test failed: the basket should be empty!\n";
        else
            cout << "Test succeeded: operator said the basket is empty!\n";

        cout << endl;
    }

    {
        printHeader("T2: Custom Constructor");

        Basket aBasket(fruits, 2, 6.99);
        cout << aBasket;

        // conversion to bool operator
        if (aBasket)
            cout << "Test succeeded: operator said the basket has content!\n";
        else
            cout << "Test failed: the basket should NOT be empty!\n";

        cout << endl;
    }

    {
        printHeader("T3: += operator");

        Basket aBasket;
        aBasket += fruits[2];
        (aBasket += fruits[0]) += fruits[4];
        aBasket.setPrice(12.234);

        cout << aBasket;
        cout << endl;
    }
    
    {
        printHeader("T4: Copy Constructor");

        Basket b1;
        Basket b2(b1);

        cout << "Basket #1 -> " << b1;
        cout << "Basket #2 -> " << b2;

        b1 += fruits[3];
        b1.setPrice(3.50);

        Basket b3(b1);
        cout << "Basket #3 -> " << b3;
        cout << endl;
    }

    {
        printHeader("T5: Copy Assignment");

        Basket b1, b2, b3(fruits, 5, 19.95);

        b1 = b2;
        cout << "Basket #1 -> " << b1;
        cout << "Basket #2 -> " << b2;

        b1 = b3;
        cout << "Basket #1 -> " << b1;

        b3 = b2;
        cout << "Basket #3 -> " << b3;
    }

    return 0;
}

Basket.h

#ifndef Basket_h
#define Basket_h

#include <stdio.h>
#include <iomanip>
#include <iostream>

namespace sdds{

struct Fruit
{
  char m_name[30 + 1]; 
  double m_qty;        
};

class Basket{
private:
    Fruit *m_fruits;
    int m_cnt;
    double m_price;
public:
    Basket(); 
    Basket(Fruit* fruits, int cnt, double price); 
    Basket(Basket &d);
    Basket& operator = (Basket &d);
    ~Basket(); 
    void setPrice(double price);
    operator bool() const; 
    Basket& operator+=(Fruit d); 
    friend std::ostream& operator << (std::ostream& output, Basket test); 
};

}
#endif /* Basket_h */

Basket.cpp

#include "Basket.h"

using namespace sdds;

namespace sdds {

Basket::Basket(){  
    m_fruits = nullptr;
    m_cnt = 0;
    m_price = 0;
}

Basket::Basket(Fruit* fruits, int cnt, double price){
    if (cnt > 0 && fruits != nullptr) { 
        m_cnt = cnt;
        m_price = price;
        m_fruits = new Fruit[cnt + 1];
        for (int i = 0; i < cnt; i++) {
            m_fruits[i] = fruits[i];
        }
    }else{
        m_fruits = nullptr;
        m_cnt = 0;
        m_price = 0;
    }
}

Basket::Basket(Basket &d){
    m_price = d.m_price;    // Shallow Copying
    m_cnt = d.m_cnt;
    m_fruits = new Fruit[m_cnt + 1];
    for (int i = 0; i < m_cnt; i++) {   // Deep Copying
        m_fruits[i] = d.m_fruits[i];
    }
}

Basket& Basket::operator = (Basket &d){     
    m_price = d.m_price;
    m_cnt = d.m_cnt;
    m_fruits = new Fruit[m_cnt + 1];
    for (int i = 0; i < m_cnt; i++) {
        m_fruits[i] = d.m_fruits[i];
    }
    return *this;
}

Basket::~Basket(){  
    delete [] m_fruits;
}

void Basket::setPrice(double price){    
    m_price = price;
}

Basket::operator bool() const {
    // returning true if the Basket is valid
    return m_fruits != nullptr;
}

Basket& Basket::operator+=(Fruit d){    
    Fruit* tmp = new Fruit[m_cnt + 1];
    for (int i = 0; i < m_cnt; i++) {
        tmp[i] = m_fruits[i];
    }
    tmp[m_cnt++] = d;
    delete [] m_fruits;
    m_fruits = tmp;
    
    return *this;
}

std::ostream& operator << (std::ostream& output, Basket test){  
    if (test.m_cnt == 0 || test.m_price == 0 || test.m_fruits == nullptr) {
        output << "The basket is empty!" << std::endl;
    }else{
        output << "Basket Content:" << std::endl;
        std::cout << std::fixed;
        std::cout << std::setprecision(2);
        for (int i = 0 ; i < test.m_cnt; i++) {
            output << std::setw(10) << test.m_fruits[i].m_name << ": " <<test.m_fruits[i].m_qty << "kg" << std::endl;
        }
        output << "Price: " << test.m_price << std::endl;
    }
    return output;
}

}
Daboss2182
  • 25
  • 5
  • 3
    What happens to the thing that `m_fruits` already points to, when you run `operator=`? – Asteroids With Wings Jul 08 '20 at 21:18
  • Glad to see a course teaching "real" C++ though, with namespaces and such like (presumably vectors come later) – Asteroids With Wings Jul 08 '20 at 21:20
  • As for how to diagnose it ... dunno, just look at all your `new`s and look at all your `delete`s, and try to spot the missing one I guess :) Memcheck _should_ tell you _what_ was lost... – Asteroids With Wings Jul 08 '20 at 21:20
  • @AsteroidsWithWings I have created a destructor to delete m_fruits, shouldnt that take of all the news ? – Daboss2182 Jul 08 '20 at 21:26
  • 2
    Unrelated: [a simple and nearly foolproof way to implement an assignment operator](https://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom). It's not always the best solution, but it makes a great place to start and you'll find that most of the time you'll stay there. – user4581301 Jul 08 '20 at 21:36
  • @Daboss2182 Assignment does not invoke the destructor first. – Asteroids With Wings Jul 08 '20 at 21:36
  • 1
    @Daboss2182 Your copy assignment operator could have been written very simply and correctly by using copy / swap. It takes about a minute to get it working, instead of duplicating the code that's in the copy constructor. – PaulMcKenzie Jul 08 '20 at 21:36

1 Answers1

3

Your assignment operator leaks memory, since you failed to delete[] the original data when doing the assignment.

The easiest way to get a working assignment operator is to use the copy/swap idiom:

#include <algorithm>
//...
Basket& Basket::operator = (const Basket &d)
{     
    if ( &d != this)
    {
       Basket temp(d);
       std::swap(temp.m_price, m_price);
       std::swap(temp.m_cnt, cnt);
       std::swap(temp.m_fruits, fruits);
    }
    return *this;
}

Your original assignment operator had multiple flaws:

  1. Did not delete[] the old memory.

  2. Did not do a check for self-assignment, thus Basket a; a = a; would fail.

  3. Changed member variables before issuing a call to new[], thus an exception thrown would corrupt your object.

All three issues are taken care of with the code shown above.

In fact, item 2) need not be done for copy / swap to work (but done in the example code above, just to show what your original code was missing).

PaulMcKenzie
  • 31,493
  • 4
  • 19
  • 38