Destructor troubles...

[Minime80]

Alright, I've got it all worked out. I've got my linked list, and it's working beautifully, and I'm loading data files into it fine. The problem happens when it hits the
return 0;
statement at the end of main, so I'm assuming this means it's running the destructor for my list class and something ain't happenin' correctly. Here's my destructor:listings::~listings()
{
// Destructor goes through list and frees all the memory

node *next; // Points to next node
next = head[0]->next[0];

while(head[0])
{
delete [] head[0]->data.house_address;
delete [] head[0]->data.house_city;
delete [] head[0]->data.house_state;
delete [] head[0]->data.house_desc;
delete [] head[0];

head[0] = next; // progress to next record
}
}It has a problem deleting the contents of head[0]->data.anything but doesn't have a problem deleting head[0] itself. Did I write something wrong or is the problem somewhere else?

[bdg1983]

You only need to delete things created with the new keyword. By the look of it, head is a pointer to a class/struct which contains a struct called data? Anyway, it doesn't look as if you've dynamically allocated anything but what head poits to, so try getting rid of all the delete lines except the one that deletes head itself.

[ 02 October 2001: Message edited by: Bradmont ]

[Minime80]

The things inside data that I'm "delete"ing are dynamically allocated char arrays so I think I need to delete them before deleting the actual node.

[Stuka]

What OS/Compiler is this on? If you're using VC++, you have to be VERY careful about where you delete dynamically allocated memory. I've heard other compilers aren't as picky.

[pinoy]

Originally posted by Stuka:
<STRONG>What OS/Compiler is this on? If you're using VC++, you have to be VERY careful about where you delete dynamically allocated memory. I've heard other compilers aren't as picky.</STRONG>

If it crashes, it's usually a bug in the code. VC++ is a relatively stable compiler, sure it has some bugs (like incorrect scope of variables declared in a for loop), but 99% of the time it generates the correct code. VC++ is picky under debug mode because it does a lot of checking, eg. uninitialized pointers are usually set to 0xcdcdcdcd, it has a custom allocator that keeps track of what has been allocated and checks for consistency.

This is not part of the compiler though, but a part of the library. This is actually a good thing, because you can spot errors much easier during development. When you're ready for releasing the code, all these checks can be disabled for efficiency reasons. GCC does not have this by default, but there are several libraries out there to help you with pointer stuff, such as Electric Fence.

MiniMe:
I know linked lists are fun to play with when you're starting out. It can help you get up to speed with pointers and memory allocation quickly, but what about just using the standard containers? Most of the top C++ programmers now actually recommend this approach. The containers can help you write effective programs quickly, and the iterators will also help you understand pointers. Once you have a good grasp of that, you can then move on to some of the more low level stuff, and implement your own data structures (eg linked list).

Also for some constructive criticism on your code. There are several things wrong with it.

1. next = head[0]-&gt;next[0];
This notation is unusual. It is the same as:
next = head-&gt;next[0];

2. head[0] = next;
next points to nowhere. Do you mean:
head = head-&gt;next;

3. Each node should be a class that has it's own destructor. And your list destructor might look something like:


listing::~listing()
{
while (head) { // assuming head is a member variable
Node* tmp = head;
head = head-&gt;next;
delete tmp;
}
}

Node::~Node()
{
delete[] house_address;
delete[] house_city;
//...
}

[pinoy]

BTW, here's a quick and dirty version of your linked list.


#include &lt;iostream&gt;
#include &lt;cstring&gt;
#include &lt;memory&gt;

using namespace std;

class Node {
friend ostream& operator&lt;&lt; (ostream& o, const Node& node);
public:
Node(const char* address, const char *city)
: next_(0)
{
address_ = strdup(address);
city_ = strdup(city);
}

~Node()
{
cout &lt;&lt; "Destroying Node: " &lt;&lt; address_ &lt;&lt; endl;
free(address_);
free(city_);
}

char* address_;
char* city_;
Node* next_;
};

class List {
public:
List()
: head_(0)
{}

~List()
{
cout &lt;&lt; "Destroying list..." &lt;&lt; endl;
while (head_) {
Node* next = head_-&gt;next_;
delete head_;
head_ = next;
}
}

void Add(const char* address, const char* city)
{
Node* node = new Node(address, city);
// add to tail
Node* ptr = head_;
if (!ptr) {
head_ = node;
return;
}
for (;ptr-&gt;next_; ptr = ptr-&gt;next_)
;
ptr-&gt;next_ = node;
}

void Display()
{
for (Node* node = head_; node; node = node-&gt;next_)
cout &lt;&lt; *node;
}

private:
Node* head_;
};

ostream& operator&lt;&lt; (ostream& o, const Node& node)
{
o &lt;&lt; "Address: " &lt;&lt; node.address_ &lt;&lt; endl;
o &lt;&lt; "City: " &lt;&lt; node.city_ &lt;&lt; endl;
o &lt;&lt; endl;
}

int main(void)
{
List list;
list.Add("My Address", "My City");
list.Add("Another address", "Another City");
list.Add("Yet another address", "Yet another city");

list.Display();
return 0;
}

[Stuka]

pinoy-
What I'm actually referring to in VC++ is the fact that it will choke (and hard) on a class that allocates memory in a member function, and tries to free it in the destructor. Now, if this is not legal in C++, I've never heard of it - it makes a ton of sense to me for that to be a standard method for freeing dynamically allocated memory. However, VC++ (6.0 at least) will NOT let you do it.

[pinoy]

Originally posted by Stuka:
<STRONG>pinoy-
What I'm actually referring to in VC++ is the fact that it will choke (and hard) on a class that allocates memory in a member function, and tries to free it in the destructor. Now, if this is not legal in C++, I've never heard of it - it makes a ton of sense to me for that to be a standard method for freeing dynamically allocated memory. However, VC++ (6.0 at least) will NOT let you do it.</STRONG>

Stuka, I've never had this problem, and we do this everyday. It's a common technique to free pointers at destructor. You might want to make sure that the pointer is initialized in the constructor to NULL. Because if it's uninitialized, you can't delete it.


Ptr* ptr = NULL;
delete ptr; // OK
ptr = new Ptr;
delete ptr; // OK

Ptr* ptr2;
delete ptr2; // Error: undefined.


Also, since you have pointers as members, it is a good guideline, to provide a copy constructor and assignment operator for the class, private or otherwise.