Solved

Cant Find (Simple?) Memory Leak in C++.

Posted on 2009-04-11
7
859 Views
Last Modified: 2013-12-17
Hi, I'm currently a C plus plus student and my assignments must be memory leak free. I've spent so long trying to figure out where this memory leak is and I have a feeling it should be really easy to find. I'm using Visual C plus plus 2008 Express Edition with its built in memory leak dumper. Someone who knows their way around C or C plus plus (which is probably more than I know) would probably find this really fast. Attached is all my code and an example of a memory leak dump.

Things to note:
1. It seems I have a memory leak for each winery I add.
2. I dont have a destructor for the private struct node definition in list.h (I dont know if this is important because I tried adding one and it didnt work. Maybe I'm doing it wrong?)
3. I also noticed that right after a new winery is inserted, the destructor for the winery class is called. I have no idea why this is or if it's important.

Thank you for your help.
//============================= list.h =====================================

#ifndef _LIST_

#define _LIST_
 

#include <ostream>

#include "winery.h"
 

using namespace std;
 

class list

{

public:

	list(void);				// constructor

	virtual ~list(void);	// destructor

	void displayByName(ostream& out) const;

	void displayByRating(ostream& out) const;

	void insert(const winery& toInsert);

	winery * const find(const char * const name) const;

	bool remove(const char * const name);
 

private:

	struct node

	{

		node(const winery& winery)

		:item(winery), nextByName(0), nextByRating(0)

		{}		// constructor

		winery item;

		node * nextByName;

		node * nextByRating;

	};
 

	node * headByName;

	node * headByRating;

};
 

#endif // _LIST_
 
 

//========================== winery.h ======================================

#ifndef _WINERY_

#define _WINERY_
 

#include <ostream>
 

class winery

{

public:

	winery(const char * const name, const char * const location, const int acres, const int rating);

	virtual ~winery(void);

	const char * const getName() const;

	const char * const getLocation() const;

	const int getAcres() const;

	const int getRating() const;
 

	// display headings for lists of wineries

	static void displayHeadings(std::ostream& out);
 

	friend std::ostream& operator<<(std::ostream& out, winery *w);
 

private:

	char	*name;

	char	*location;

	int		acres;

	int		rating;

};
 

#endif // _WINERY_
 
 

//================================== list.cpp =============================

#include "list.h"

#include "winery.h"

#include <cstring>
 

list::list()

:headByName (0), headByRating(0)

{

	// your code here

}
 

list::~list()

{

	do{

		node *temp;

		temp = headByName;

		headByName = headByName->nextByName;

		delete temp;

	}while(headByName);

}
 

void list::displayByName(ostream& out) const

{
 

	node *tempNode;

	tempNode = headByName;

	winery::displayHeadings(out);

	while(tempNode != 0){

        out<<&tempNode->item;

        tempNode = tempNode->nextByName;

	}

	// your code here
 

}
 

void list::displayByRating(ostream& out) const

{
 

	node *tempNode;

	tempNode = headByRating;

	winery::displayHeadings(out);

	while(tempNode != 0){

        out<<&tempNode->item;

        tempNode = tempNode->nextByRating;

	}

	// your code here
 

}
 

void list::insert(const winery &toInsert)

{

	// your code here

	winery *theWinery = new winery (toInsert.getName(), toInsert.getLocation(), toInsert.getAcres(), toInsert.getRating());

	

	node *theNode, *tempNode, *tempNode2;

	bool nameIn = false, rateIn = false;
 

	theNode = new node(*theWinery);
 

	if (headByName == 0){

	    headByName = theNode;

	    headByRating = theNode;

	    return;

	}else{

	    tempNode = headByName;

	    if(strcmp(theWinery->getName(), tempNode->item.getName()) < 0){

            //new node becomes the new first node.

            theNode->nextByName = headByName;

            headByName = theNode;

            nameIn = true;

        }
 

		while(!nameIn && tempNode->nextByName != NULL){

            if(strcmp(theWinery->getName(), tempNode->item.getName()) > 0 && strcmp(theWinery->getName(), tempNode->nextByName->item.getName()) < 0){

                //new node goes between current node and next node

                theNode->nextByName = tempNode->nextByName;

                tempNode->nextByName = theNode;

                nameIn = true;

            }else{

                tempNode = tempNode->nextByName;

            }

        }
 

        tempNode2 = headByRating;

		if(theWinery->getRating() > tempNode2->item.getRating()){

            //new node becomes the new first node.

            theNode->nextByRating = headByRating;

            headByRating = theNode;

            rateIn = true;

        }

                        

        while(!rateIn && tempNode2->nextByRating != NULL){

            if(theWinery->getRating() < tempNode2->item.getRating() && theWinery->getRating() > tempNode2->nextByRating->item.getRating()){

                //new node goes between current node and next node

                theNode->nextByRating = tempNode2->nextByRating;

                tempNode2->nextByRating = theNode;

                rateIn = true;

            }else{

                tempNode2 = tempNode2->nextByRating;

            }

        }
 

        if(!rateIn){

            tempNode2->nextByRating = theNode;

        }

        if(!nameIn){

            tempNode->nextByName = theNode;

        }

        return;

	}
 

}
 

winery * const list::find(const char * const name) const

{

	// your code here, return the appropriate value

	node * tempNode = headByName;

	while(tempNode != 0){

        if(strcmp(name, tempNode->item.getName()) == 0){

            node * const constNode = tempNode;

            return &constNode->item;

        }else{

            tempNode = tempNode->nextByName;

        }

	}

	return 0;

}
 

bool list::remove (const char * const name)

{

	// your code here, return the appropriate value

	

	node *tempNode, *tempNode2;

	tempNode = headByName;

	bool removed = false;
 

	if(strcmp(name, tempNode->item.getName()) == 0){

        headByName = tempNode->nextByName;

        removed = true;

    }

	while(!removed && tempNode->nextByName != 0){

        if(strcmp(name, tempNode->item.getName()) == 0){

            tempNode2->nextByName = tempNode->nextByName;

            removed = true;

        }else{

            tempNode2 = tempNode;

            tempNode = tempNode->nextByName;

        }

	}
 

	if(!removed && strcmp(name, tempNode->item.getName()) == 0){

        tempNode2->nextByName = 0;

        removed = true;

    }

    if(removed){

        if(headByRating == tempNode){

            headByRating = headByRating->nextByRating;

            delete tempNode;

        }else{

            tempNode2 = headByRating;

            while(tempNode2->nextByRating != 0){

                if(tempNode2->nextByRating == tempNode){

                    tempNode2->nextByRating = tempNode2->nextByRating->nextByRating;

                    delete tempNode;

                    break;

                }else{

                    tempNode2 = tempNode2->nextByRating;

                }

            }

        }

    }
 

	return removed;

}
 
 

//=================================== winery.cpp ==========================

#include <cstring>

#include "winery.h"
 

using namespace std;
 

winery::winery(const char * const name, const char * const location, const int acres, const int rating)

: name (new char[strlen(name)+1]), location (new char[strlen(location)+1]), acres (acres), rating (rating)

{

	strcpy(this->name, name);

	strcpy(this->location, location);

	// your code here, or in this constructor's initialization list

}
 

winery::~winery()

{

	delete [] this->name;

	delete [] this->location;

}
 

const char * const winery::getName() const

{

	// your code here, return the appropriate value

	return this->name;

}
 

const char * const winery::getLocation() const

{

	// your code here, return the appropriate value

	return this->location;

}
 

const int winery::getAcres() const

{

	// your code here, return the appropriate value

	return this->acres;

}
 

const int winery::getRating() const

{

	// your code here, return the appropriate value

	return this->rating;

}
 

void winery::displayHeadings(ostream& out)

{

	out.width(26);

	out<<left<<"name";

	out.width(19);

	out<<left<<"location";

    out.width(5);

	out<<right<<"acres";

	out.width(7);

	out<<right<<"rating";

	out.width(26);

	out<<endl<<left<<"----";

	out.width(19);

	out<<left<<"--------";

    out.width(5);

	out<<right<<"-----";

	out.width(7);

	out<<right<<"------";

	out<<endl;

	// your code here

}
 

ostream& operator<<(ostream& out, winery *w)

{

	// your code here

	out.width(26);

	out<<left<<w->getName();

	out.width(19);

	out<<left<<w->getLocation();

	out.width(5);

	out<<right<<w->getAcres();

	out.width(7);

	out<<right<<w->getRating()<<endl;

	return out;

}
 
 

//================================= lab1driver.cpp ===========================

#ifdef _WIN32

#define _CRTDBG_MAP_ALLOC

#include <stdlib.h>

#include <crtdbg.h>

#endif
 

#include <iostream>
 

#include "winery.h"

#include "list.h"
 

using namespace std;
 

int main()

{

    list	*wineries = new list();

	winery	*wPtr;
 

    cout << "\nCS260 - Lab1 - Your Name" << endl << endl;
 

	wineries->insert(winery("Lopez Island Vinyard", "San Juan Islands", 7, 95));

	wineries->insert(winery("Gallo", "Napa Valley", 200, 25));

	wineries->insert(winery("Cooper Mountain", "Willamette Valley", 100, 47));

	wineries->insert(winery("Duck Pond Cellars", "Willamette Valley", 845, 70));

	wineries->insert(winery("Del Rio", "Bear Creek Valley", 200, 37));

	wineries->insert(winery("Weisinger's of Ashland", "Bear Creek Valley", 6, 83));

	wineries->insert(winery("LongSword", "Applegate Valley", 16, 85));
 

	cout << "\n+++ list by name\n";

	wineries->displayByName(cout);

	cout << "\n+++ list by rating\n";

	wineries->displayByRating(cout);
 

	cout << "\n>>> removing Cooper Mountain\n";

	wineries->remove("Cooper Mountain");
 

	cout << "\n+++ list by name\n";

	wineries->displayByName(cout);

	cout << "\n+++ list by rating\n";

	wineries->displayByRating(cout);
 

	cout << "\n>>> inserting San Juan Vinyards\n";

	wineries->insert(winery("San Juan Vinyards", "San Juan Islands", 20, 90));
 

	cout << "\n+++ list by name\n";

	wineries->displayByName(cout);

	cout << "\n+++ list by rating\n";

	wineries->displayByRating(cout);
 

	cout << "\n>>> search for \"Gallo\"\n\n";

	wPtr = wineries->find("Gallo");

	if (wPtr != 0)

		cout << wPtr;

	else

		cout << "not found" << endl;
 

	cout << "\n>>> search for \"No Such\"\n\n";

	wPtr = wineries->find("No Such");

	if (wPtr != 0)

		cout << wPtr;

	else

		cout << "not found" << endl;
 

	cout << endl;

	delete wineries;

	

// in Visual Studio, reports on memory leaks in the Output window

#ifdef _WIN32

	_CrtDumpMemoryLeaks();

#endif

	cin.get();
 

	return 0;

}
 
 
 

//=================== example of memory leak dump after execution ===============

Detected memory leaks!

Dumping objects ->

{183} normal block at 0x003364A8, 20 bytes long.

 Data: <  A  d3 He3     > 18 9A 41 00 F8 64 33 00 48 65 33 00 14 00 00 00 

{157} normal block at 0x00336978, 20 bytes long.

 Data: <  A  i3  j3     > 18 9A 41 00 C8 69 33 00 10 6A 33 00 10 00 00 00 

{151} normal block at 0x00336828, 20 bytes long.

 Data: <  A xh3  h3     > 18 9A 41 00 78 68 33 00 D0 68 33 00 06 00 00 00 

{145} normal block at 0x003366E8, 20 bytes long.

 Data: <  A 8g3  g3     > 18 9A 41 00 38 67 33 00 80 67 33 00 C8 00 00 00 

{139} normal block at 0x003365A0, 20 bytes long.

 Data: <  A  e3 @f3 M   > 18 9A 41 00 F0 65 33 00 40 66 33 00 4D 03 00 00 

{133} normal block at 0x00336458, 20 bytes long.

 Data: <  A  d3  d3 d   > 18 9A 41 00 A8 64 33 00 F8 64 33 00 64 00 00 00 

{127} normal block at 0x00336320, 20 bytes long.

 Data: <  A pc3  c3     > 18 9A 41 00 70 63 33 00 B8 63 33 00 C8 00 00 00 

{121} normal block at 0x003361D0, 20 bytes long.

 Data: <  A  b3 xb3     > 18 9A 41 00 20 62 33 00 78 62 33 00 07 00 00 00 

Object dump complete.

Open in new window

0
Comment
Question by:nlhess2003
  • 3
  • 3
7 Comments
 
LVL 45

Expert Comment

by:Kdo
ID: 24123951
Hi nlhess2003,

It appears that when you add an item to the list, you instantiate both winery and node objects.

When you remove an item from the list, you delete the node, but not the winery object.



Good Luck,
Kent
0
 

Author Comment

by:nlhess2003
ID: 24123978
I did notice this, but I'm not sure how to delete the winery object.
I tried this for my list destructor:
list::~list()
{
      do{
            node *temp;
            temp = headByName;
            headByName = headByName->nextByName;
            delete temp->item;               //But the winery in this node is not a pointer. So this doesnt work.
            delete temp;
      }while(headByName);
}
I also tried delete &temp->item;  but that just made my program crash.
0
 

Author Comment

by:nlhess2003
ID: 24123989
Also I noticed that when temp is deleted in the list destructor, the winery destructor is being called right after. I think this is supposed to happen but I'm not sure how it is.
0
DevOps Toolchain Recommendations

Read this Gartner Research Note and discover how your IT organization can automate and optimize DevOps processes using a toolchain architecture.

 
LVL 4

Accepted Solution

by:
lhl60 earned 500 total points
ID: 24133560
As kdo points out your problem is here

void list::insert(const winery &toInsert)
{
      // your code here
      winery *theWinery = new winery (toInsert.getName(), toInsert.getLocation(), toInsert.getAcres(),
..
..
}

theWinery is never deleted,

winery is a pointer everywhere in the code except in  node, so  why not change
      
struct node
      {
            node(const winery& winery)
            :item(winery), nextByName(0), nextByRating(0)
            {}            // constructor
            winery item;
            node * nextByName;
            node * nextByRating;
      };

to
      struct node
      {
           node(const winery* winery)
            :item(winery), nextByName(0), nextByRating(0)
            {}            // constructor
            winery *item;
            node * nextByName;
            node * nextByRating;
~node(){delete item;}
};

void list::insert(const winery &toInsert)   to
void list::insert(const winery *toInsert)

you might have to change a few "."' to "->"' and delete some &





0
 
LVL 4

Expert Comment

by:lhl60
ID: 24133578
continued...

will also make this part more redable

void list::insert(const winery &toInsert)
{
      // your code here
      winery *theWinery = new winery (toInsert.getName(), toInsert.getLocation(), toInsert.getAcres(), toInsert.getRating());
      
      node *theNode, *tempNode, *tempNode2;
      bool nameIn = false, rateIn = false;
 
      theNode = new node(theWinery);

0
 

Author Comment

by:nlhess2003
ID: 24134045
Thank you so much lhl60. In the end I decided that I will just have to change my professor's code. It just makes more sense to use a pointer there. Thanks again.
0
 
LVL 4

Expert Comment

by:lhl60
ID: 24136422
no problem, glad to help
good luck
0

Featured Post

3 Use Cases for Connected Systems

Our Dev teams are like yours. They’re continually cranking out code for new features/bugs fixes, testing, deploying, testing some more, responding to production monitoring events and more. It’s complex. So, we thought you’d like to see what’s working for us.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Suggested Solutions

Title # Comments Views Activity
Assigning default value to structure in C for mutithread application 17 57
Memory Usage 2 50
Linq Help 1 35
Name space syntax error 12 43
Update (December 2011): Since this article was published, the things have changed for good for Android native developers. The Sequoyah Project (http://www.eclipse.org/sequoyah/) automates most of the tasks discussed in this article. You can even fin…
Real-time is more about the business, not the technology. In day-to-day life, to make real-time decisions like buying or investing, business needs the latest information(e.g. Gold Rate/Stock Rate). Unlike traditional days, you need not wait for a fe…
Video by: Grant
The goal of this video is to provide viewers with basic examples to understand and use while-loops in the C programming language.
The goal of this video is to provide viewers with basic examples to understand opening and reading files in the C programming language.

920 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

15 Experts available now in Live!

Get 1:1 Help Now