Dynamic memory, free()

[kevinstrijbos]

Hello,

I'm trying to write a program which deletes the last element in a linked list of structures. However, I'm stuck with free, I know how it works but free() always give problems, any help?

This is my code:

/* deletes last element */
void delete_last (LINKED* *list)
{
	LINKED *p;
	LINKED *q;


	p = *list; // initialize pntr
	
	if (*list == NULL) // control for empty list
		printf("EMPTY LIST.\n");
	else if ((*list)->pntr == NULL) // control for 1 element
		free((*list)->pntr);
	else
	{
		while (p->pntr != NULL)
		{
			q = p;
			p = p->pntr;
		}


		free(q);
		q = NULL;
	}
}

[laserlight]

If there is only one element, then you should be freeing the memory for that element and then set the pointer to that element to be a null pointer. If there is more than one element, then you should be freeing the memory for the last element, then set the next pointer for the previous element to be a null pointer.

[kevinstrijbos]

Oh yes, I see the problem when I have one element. But with more elements it should work, doesn't it?

[laserlight]

Yeah, since the loop should iterate at least once, though I'm still uncomfortable not initialising q first. (Naming q previous might be better, and what's with pntr when next would be a much better name?)

That said, note that setting q to be a null pointer only changes the pointer local to the function.

[kevinstrijbos]

No, q also changes the value in list. Note that p = *list, and after that q = p.
And indeed, it'd be better to initialise q, also the names could be better.
But the matter is that the loop doesn't work...

[laserlight]

No, q also changes the value in list. Note that p = *list, and after that q = p.
(...)
But the matter is that the loop doesn't work...

You're free to believe what you want to believe. All that will happen is that the loop will continue to appear to not work

EDIT:
Actually, I suspect that I'm also getting confused as to what is p and q. Silly undescriptive names. I'll write this thing out myself to make it clearer.

[kevinstrijbos]

Well, after a lot of trouble with ctrl+f I managed to change the names!

[laserlight]

Oh, okay. I understand now. Indeed, this is wrong:

free(q);
q = NULL;

It should be:

free(p);
q->nptr = NULL;

free(q) is wrong because q is the previous element. It is not the last element. The assignment of NULL to q is clearly insufficient because q is a local variable. Yes, it points to an element in the linked list, but setting it to be a null pointer just means that it no longer points to an element in the linked list. It does not change any element in the linked list.