Rule of three memory leaks

[d0253]

==901== Invalid free() / delete / delete[] / realloc()
==901== at 0x4C3029C: operator delete(void*) (vg_replace_malloc.c:576)
==901== by 0x400FDE: Grocery::~Grocery() (Grocery.cpp:33)
==901== by 0x401720: main (lab11.cpp:22)
==901== Address 0x5b4a0c8 is 8 bytes inside a block of size 168 alloc'd
==901== at 0x4C2F933: operator new[](unsigned long) (vg_replace_malloc.c:423)
==901== by 0x400EDA: Grocery::Grocery(int) (Grocery.cpp:8)
==901== by 0x401373: main (lab11.cpp:5)
==901==
==901== Invalid free() / delete / delete[] / realloc()
==901== at 0x4C3029C: operator delete(void*) (vg_replace_malloc.c:576)
==901== by 0x400FDE: Grocery::~Grocery() (Grocery.cpp:33)
==901== by 0x40172F: main (lab11.cpp:15)
==901== Address 0x5b4a0c8 is 8 bytes inside a block of size 168 alloc'd
==901== at 0x4C2F933: operator new[](unsigned long) (vg_replace_malloc.c:423)
==901== by 0x400EDA: Grocery::Grocery(int) (Grocery.cpp:8)
==901== by 0x401373: main (lab11.cpp:5)
==901==
==901== Invalid free() / delete / delete[] / realloc()
==901== at 0x4C3029C: operator delete(void*) (vg_replace_malloc.c:576)
==901== by 0x400FDE: Grocery::~Grocery() (Grocery.cpp:33)
==901== by 0x40173E: main (lab11.cpp:5)
==901== Address 0x5b4a0c8 is 8 bytes inside a block of size 168 alloc'd
==901== at 0x4C2F933: operator new[](unsigned long) (vg_replace_malloc.c:423)
==901== by 0x400EDA: Grocery::Grocery(int) (Grocery.cpp:8)
==901== by 0x401373: main (lab11.cpp:5)
==901==
==901==
==901== HEAP SUMMARY:
==901== in use at exit: 168 bytes in 1 blocks
==901== total heap usage: 3 allocs, 5 frees, 73,896 bytes allocated
==901==
==901== LEAK SUMMARY:
==901== definitely lost: 168 bytes in 1 blocks
==901== indirectly lost: 0 bytes in 0 blocks
==901== possibly lost: 0 bytes in 0 blocks
==901== still reachable: 0 bytes in 0 blocks
==901== suppressed: 0 bytes in 0 blocks
==901== Rerun with --leak-check=full to see details of leaked memory
==901==
==901== For counts of detected and suppressed errors, rerun with: -v
==901== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)





#include "Grocery.h"

//Overloaded constructor
Grocery::Grocery(int cap) {
cout << "Grocery Constructor: Capacity=" << cap << endl;
m_capacity = cap;
m_used = 0;
m_groceryList = new string[cap];
}

//Implement Copy Constructor Here

Grocery::Grocery(const Grocery& p2){
m_capacity = p2.m_capacity;
m_used = p2.m_used;
m_groceryList = p2.m_groceryList;
}
//Implement Overloaded Assignment Operator Here

Grocery& Grocery:: operator = (const Grocery& source){
m_capacity = source.m_capacity;
m_used = source.m_used;
m_groceryList = source.m_groceryList;
return *this;
}


//Implement Destructor Here

Grocery::~Grocery (){
//m_capacity = m_used = 0;
//*m_groceryList = "";
delete[] m_groceryList;
//m_groceryList = NULL;

}

//Inserting a new subject into our subject array
void Grocery::insert(const string& itemName) {
if(m_used == m_capacity) {
cout<<"Grocery List is full. Cannot add any additional items."<<endl;
}
else {
m_groceryList[m_used] = itemName;
m_used++;
cout << "Item:" << itemName << " added to the grocery list." << endl;
}
}

void Grocery::deleteLast(){
if (m_used == 0){
cout <<"Grocery List Empty!" <<endl;
}
else{
m_used--;
cout <<"Item:" <<m_groceryList[m_used] <<" removed from grocery list." << endl;
m_groceryList[m_used] = "";
}
ostream& operator <<(ostream& os, const Grocery& rg) {
os << "m_capacity: " <<rg.m_capacity <<endl;
os << "m_used: " <<rg.m_used <<endl;

os << "Your list has selected the following items:" << endl << " | ";
for(int i=0;i<rg.m_used; i++) {
os <<rg.m_groceryList[i] + " | ";
}
return os << endl;
}

[Repeater]

m_groceryList = source.m_groceryList;
Two different Grocery objects pointing to the same data. Each of those two will call delete on the data. This is very bad.

Basically, your copy constructor is wrong and so is your assignment. You're not copying the data. You're just copying the pointer. You need to copy the data.

[ne555]

or apply rule of zero
std::vector<std::string> m_groceryList;
now you don't need a custom destructor, copy constructor or assignment operator

[d0253]

How do I fix this?

[Thomas1965]

Just follow ne555's advice.
The golden rule is "Don't use pointers and dynamic memory - unless you have no other choice."

[d0253]

I have to use it.

[ne555]

for the assignment operator use the copy-and-swap idiom

Grocery& Grocery::operator=(Grocery source){
   //note that the parameter is passed by copy, so it calls the copy constructor
   swap(*this, source);
   return *this;
}

friend void Grocery::swap(Grocery &a, Grocery &b){
   using std::swap;
   swap(a.m_capacity, b.m_capacity);
   swap(a.m_used, b.m_used);
   swap(a.m_groceryList, b.m_groceryList);
}

//all at once, if you prefer
Grocery& Grocery::operator=(Grocery source){
   this->m_capacity = source.m_capacity;
   this->m_used = b.m_used;
   this->m_groceryList = source.m_groceryList; //yes, just the pointer (the copy-constructor already did the job)

   //as we'll destroy `source' we don't care what it holds, except for
   source.m_groceryList = nullptr; //avoid deleting the pointer twice
   return *this;
}

now, for the copy constructor, copy each element one by one (like you would copy two arrays)