running off the end of the list

I'm running off the list on my destructor.
Can someone tell me how to stop the while loop before this happens.

I made some adjustements to a prior posted code in regards to my program, but now when i'm destructing my list it seems like the while loop keeps running to far and goes off the list into garbage.

Is there a test i'm missing?

Code:

	linklist::~linklist()
	{
		link *current;
		
		if(head!=NULL)
		{
			current = head->next;

			while (current != NULL)  
				{
					x++;
					cout << "delete node " << head->lname << endl;
					delete head;
					head = current;
					current = current->next;
				}

			cout << "Deleting last node " << head->lname << endl;
			delete head;
		}
		else
		{
			delete head, tail;
		}

		head=NULL;
		tail=NULL;
		current=NULL;
	}

how were the nodes allocated? using malloc or new?

easier way:

Code:

link *curr;
	while(head != NULL) {
		curr = head->next;
		free(head);
		head = curr;
	}
	free(curr);

(ripped it out of code that i have)
allocated using new

Other than the fact that there is an easier way like the Wookie showed, your code doesn't look wrong by itself.

Provide sample input that leads to this problem.

I'm starting to think that it isn't the program running off the end of the list, but I believe it may be a prior issue I still haven't resolved. I attempted to make a copy constructor and an overloaded = to duplicate my list. When the destructor is called to destroy the created lists it makes it through the first list and then crashes out on the second i believe.

see the below output.

Build and Display a linked list
1) Create A
2) Create B
3) Append A to B
4) Append B to A
5) Merge A and B
6) Print a List
7) Detroy a list
8) Write to a File
9) Read from a File
=) Duplicates using overloading with List A
space) Allows you to use a copy constructor on list B
ESC)Quit this crap
delete node smith
Deleting last node doe
delete node �¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ �¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ
�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ �¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ ƒÃ�¡ƒÃ�¡ƒÃ
�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡ƒÃ�¡▬

Originally posted by jlou
Provide sample input that leads to this problem.

Could you provide the sample input (including how you create the lists)?

Ha! I just saw your Debugger question. I was about to ask if you knew how to use the debugger yet. Sorry, I don't know of any online information on it. I just learned from others and by playing with different things.

As for how to duplicate the error.

you need to read in list A
then attempt to copy it with the overloaded = by pressing (=)
then try to exit to see the error in the destructor by pressing (esc)
error:
Gives an assert error with the following exception.
Unhandled exception at 0x0041c166 in linked lists.exe: User breakpoint.

next read list B
then attempt to copy that one by copy constructor by pressing (spacebar)
then try to exit to see the error in the destructor by pressing (esc)
error:
Gives an assert error with the following exception.
Unhandled exception at 0x0041c166 in linked lists.exe: User breakpoint.

i connected the files in the attached .zip
(membersA.dat and membersB.dat)

Your problem is your operator= and copy constructor. It looks like you didn't quite get your last problem fixed, and that is what is causing this problem.

Because you didn't change your copy constructor and operator= to use references like was suggested, they are not being used at all. When you do listQ = listA, the compiler is using the default operator= - NOT the one you tried to write. For pointer member variables, the default operator= simply copies the pointer. Because of this, after you assign listA to listQ, both have pointers to the same list nodes. Then, listA gets destructed - deleting the list nodes, and then listQ gets destructed and tries to delete the same pointers - BAM!

The solution of course is to fix your copy constructor and your operator=.

Here is the declaration you should be using for each:

Code:

    linklist(const linklist&);//copy constructor
    linklist& operator=(const linklist&);//operator=

It shouldn't be too hard to update your code to use the references instead of pointers. Basically, instead of using '->', you use '.' like its a regular object.

why does the way that I pass make a difference.
I'm using another instansiation of the object to put the data into.
I'm extracting it from object one and placing it into object 2 that has it's own pointers, node, etc. defined? If I'm using pointers that are redefined in the processing and using object specific pointers then object 1 and object 2 do not share addresses in any way. Is this incorrect? I have tried it both ways, with & and *, I was unable to get it to compile with the &.

Well, in your code (which I don't have in front of me) you do something like this:

Code:

linklist listA;
linklist listQ;
// ...
listQ = listA;

The last line is the same as:

Code:

listQ.operator=(listA);

Now, listA is a variable of type linklist. When passing it to a method, the argument to the method can be linklist, linklist&, const linklist&, or any other type that can be implicitly converted to from your linklist (I don't think you have any of those so don't worry about that one). So if you pass listA as an argument to a method, that method better take one of those three things as a parameter.

You couldn't call a function like this:

Code:

void Foo1(int i) { cout << i << endl; }

int main()
{
    int* pi = new int(2);
    Foo(pi);  // ERROR - Type mismatch, function takes int not int*.
}

But you of course could do this:

Code:

void Foo1(int i) { cout << i << endl; }
void Foo2(int& i) { cout << i << endl; }
void Foo3(const int& i) { cout << i << endl; }
int main()
{
    int myI = 2;
    Foo1(myI);  // Fine.
    Foo2(myI);  // Fine.
    Foo3(myI);  // Fine.
}

The same goes for your operator= function inside your linklist class. If you wanted to be crafty, you could probably set something up so that you could pass in a pointer, but that would be stupid because it is easy to use the reference and it is the standard way of doing operator= and the copy constructor. Don't try and do it some crazy way because there is a reason people don't implement those functions with pointer parameters.

The reason this causes problems in your program is because your pointer versions of this function are never called. The compiler looks for an operator= function that takes one of the three parameter types I mentioned above and can't find one, so it uses the default operator= which copies the pointers and not the data, eventually leading to your problems inside the destructor.

So take the function prototypes I gave in my last post and figure out how to fix the compile errors. It really shouldn't be very hard. Like I said then, pretty much all you have to do is use '.' instead of '->' when referring to the list that is passed in (the source list).

Jlou,

Thanks for the explanation. Before I was unable to comprehend why I couldn't do it my way. And why no one suggested to do it that way. I will attempt to make the adjustments later tonight.

You said you couldn't get the copy constructor and operator= to work after changing them to use references.

Can you post the new code for those two functions only, as well as the class declaration.