Infinite Loop in Destructor...

**P3nGu1N** · 03-06-2009

Originally Posted by laserlight

It can be, or at least 0 == NULL is true.

Well, I've known '\0' == NULL, but 0 == NULL? I miss programming in Java...

**laserlight** · 03-06-2009

Originally Posted by P3nGu1N

Well, I've known '\0' == NULL, but 0 == NULL?

You should deduced that transitively since '\0' == 0

**MK27** · 03-06-2009

Originally Posted by P3nGu1N

I miss programming in Java...

Maybe it would have been easier if you had missed programming in Java

lemme know if I was right about my last post

**P3nGu1N** · 03-06-2009

Originally Posted by MK27

It looks like you just want to iterate through the array, which would mean the while() inside the for() is superfluous. I think you should choose one or the other.

you made a very valid point... I'm really just trying to destroy each list item in an array of lists, which means I should only have to access each Contents[i] once provided it wasn't NULL to begin with, then set my pointer p to the beginning of the list only in that senario, which could be done with an if statement. Changed code to:

Code:

void DestroySymTab(struct SymTab *ATable) {
	int i;
	struct SymEntry *p;
	
	for (i = 0; i < ATable->Size; i++) {
		if(ATable->Contents[i] != NULL) {
			p = (struct SymEntry*)ATable->Contents[i];
			while(p->Next != NULL){
				p = p->Next;
			}
			free(p);
			p = NULL;
		}
	}
	free(ATable);
};

and no more infinite loop. I just hope it does what I want.

**matsp** · 03-06-2009

Originally Posted by P3nGu1N

Alright, just tried this:

Code:

void DestroySymTab(struct SymTab *ATable) {
	int i;
	struct SymEntry *p;
	
	for (i = 0; i < ATable->Size; i++) {
		while(ATable->Contents[i] != NULL) {
			/*p = (struct SymEntry*)ATable->Contents[i];*/
			printf("address (Contents[i]): %p\n",ATable->Contents[i]); fflush(stdout);
			/*while(p->Next != NULL){
				p = p->Next;
			}*/
			free(p);
			p = NULL;
		}
	}
	free(ATable);
};

and it keeps spitting out address (Contents[i]): ffe0b18, which is a valid memory address I'm guessing.

If that's not how I set p to point to Contents[i], is it supposed to be *p = (struct SymEntry*)ATable->Contents[i]; instead of just p = (struct SymEntry*)ATable->Contents[i]; ?

I'm seriously bad at this stuff, sorry if you feel like I'm wasting your time.

This code is absolutely broken, since it's not setting p to anything sensible, and thus tries to free "garbage".

--
Mats

**P3nGu1N** · 03-06-2009

The sad thing is I've already created this program in Java last year. It's much easier for me to look at abstract data types like chain bucket hashing from an object oriented language.

**matsp** · 03-06-2009

Ok, so the new code only has one slight problem: You probably want to destroy EVERY element in the linked list, not just the last one.

--
Mats

**P3nGu1N** · 03-06-2009

Originally Posted by matsp

This code is absolutely broken, since it's not setting p to anything sensible, and thus tries to free "garbage".

--
Mats

What I'm trying to do is free the contents of a list of SymEntry s in the array associated with Contents of a SymTab. If I can't use a pointer, p to point to the address location where each SymEntry is, or if I'm doing it wrong, then how do I go about it?

**P3nGu1N** · 03-06-2009

Originally Posted by matsp

Ok, so the new code only has one slight problem: You probably want to destroy EVERY element in the linked list, not just the last one.

--
Mats

That's true, but how do I access the previous SymEntrys in the list after I destroy the last one? I was trying that outer while loop to destroy the last one, then the new last one, until the first SymEntry was freed, but that didn't work out so well for me.

**matsp** · 03-06-2009

It is fine to use the code you have now - the code you posted that was printing stuff, you never set p to anything.

Your latest code looks OK, except that you only free the last pointer of the linked list - which is probably not what you want to do. Note that you will need a second pointer to hold the next pointer when you delete the current one.

--
Mats

**MK27** · 03-06-2009

I see a problem!

Originally Posted by P3nGu1N

Code:

void DestroySymTab(struct SymTab *ATable) {
	int i;
	struct SymEntry *p;
	
	for (i = 0; i < ATable->Size; i++) {
		if(ATable->Contents[i] != NULL) {
			p = (struct SymEntry*)ATable->Contents[i];
			while(p->Next != NULL){
				p = p->Next;
			}
			free(p);
			p = NULL;
		}
	}
	free(ATable);
}; <- also, what is this??!!

What you are doing there is looping thru until p->Next==NULL, then you free p it and set it to NULL. Meaning the last node, not all of them. To do what you want to do I think you will need a tmp pointer:

Code:

                        while(p->Next != NULL){
                                tmp=p;
				p = p->Next;
                                free(tmp); tmp=NULL;     /* the previous ones */
			}
                        free(p); p=NULL;  /* the last one */

**matsp** · 03-06-2009

Originally Posted by P3nGu1N

That's true, but how do I access the previous SymEntrys in the list after I destroy the last one? I was trying that outer while loop to destroy the last one, then the new last one, until the first SymEntry was freed, but that didn't work out so well for me.

Ok, so we're crossing posts. Just have another variable to hold the next pointer, and set the current to next when you have deleted current - that way, you don't have to worry about it.

--
Mats

**tabstop** · 03-06-2009

Originally Posted by P3nGu1N

What I'm trying to do is free the contents of a list of SymEntry s in the array associated with Contents of a SymTab. If I can't use a pointer, p to point to the address location where each SymEntry is, or if I'm doing it wrong, then how do I go about it?

That's exactly what you want to do -- set p to point to each location you want to free and then free it. Currently you are setting p to nothing in particular, since you commented out any and every assignment involving p. Your original mistake was thinking that if you did something like this:

Code:

struct whatever *p = &foo;
p = 5;

that foo was later 5 (i.e., you assumed that contents[i] changed because you changed p). Assigning a pointer doesn't bind a reference, it just stores that address.

A point to remember is that you can free in any order; i.e. you can do something like:

Code:

while (next_to_free) is not NULL
    to_be_freed = next_to_free;
    next_to_free = next_to_free->next;
    free(to_be_freed)

**matsp** · 03-06-2009

Or this:

Code:

                        while(p != NULL){
                                tmp=p;
				p = p->Next;
                                free(tmp); 
			}

I removed the "tmp = NULL" and "p = NULL", as if you us correct scopign for them, it shouldn't be necessary to set them to NULL to avoid invalid access.

--
Mats

**MK27** · 03-06-2009

Originally Posted by matsp

I removed the "tmp = NULL" and "p = NULL", as if you us correct scopign for them, it shouldn't be necessary to set them to NULL to avoid invalid access.

Yeah, I thought about that -- since you have no way of accessing the pointers now, it doesn't matter if they are NULL, and they have been freed.

However, if you are just starting out with C I would say setting a pointer to NULL after a free() is a good habit to get into now, as in other circumstances it makes it easier to avoid a double free

Code: