CODEWITHSUNDEEP
clinked-list
radu-matei
radu-matei

Reputation: 3520

Delete the last node in a linked list

I am trying to delete the last node in a linked list, but I seem to have some difficulties. Here is the code.

void sterge_ultimul(element *lista) {
  element *p,*q;
  while (p->urmator->urmator!=NULL)
    p=p->urmator;
  q=p->urmator;
  p->urmator=NULL;
  free(q);
}

English:

void delete_last(element *list) {
   component *p, *q;
   while (p->next->next != NULL)
     p = p->next;
   q = p->next;
   p->next = NULL;
   free(q);
}

I already did the function to remove the head of the list, so I don't think I need to worry about that.

It doesn't run, and if you could help, I would appreciate.

Thanks, Radu

Upvotes: 0

Views: 174

Answers (4)

anshuman
anshuman

Reputation: 98

You didn't initialize p pointer. Its value is indeterminate.

Reading an uninitialized object is undefined behavior. Undefined behavior means anything can happen. Anything includes that your program can very well crash.

for C90, see 3.16 in the definition of undefined behavior, for C11, see 6.3.2.1p2 and for C99, see the C Committee answer in DR#338.

C90

undefined behavior: Behavior upon use [...] or of indeterminately valued objects [...]

(C11, 6.3.2.1p2)

"If the lvalue designates an object of automatic storage duration that could have been declared with the register storage class (never had its address taken), and that object is uninitialized (not declared with an initializer and no assignment to it has been performed prior to use), the behavior is undefined."

Below are the correct functions : 

void sterge_ultimul(element *lista) 
{
  element *p = lista , *q;
  while (p->urmator->urmator!=NULL)
    p=p->urmator;
  q=p->urmator;
  p->urmator=NULL;
  free(q);
}

English

void delete_last(element *list) 
{
  component *p = list, *q;
  while (p->next->next != NULL)
    p = p->next;
  q = p->next;
  p->next = NULL;
  free(q);
}

A general advice : Always make sure you have a strong reason when you see uninitialized pointers ( or variables and objects in general) in your code. This little check will save a lot of trouble for you debugging your code in future.

Upvotes: 1

alain
alain

Reputation: 12057

I would implement it like this:

void sterge_ultimul(element** p)
{        
    while(*p && (*p)->urmator)
        p = &(*p)->urmator;

    free(*p);
    *p = 0;
}

With this version you can also delete the last element, when there is only one element in the list. It should be called with the address of the pointer to the list:

sterge_ultimul(&lista);

If the list only contains one element, the last element is deleted, and lista will be 0 after the call.

Upvotes: 2

MicroVirus
MicroVirus

Reputation: 5487

Your p is never initialised, so it points to a random address in memory, and then you access it causing undefined behaviour.

What you want is to initialise p with the first element of the list (lista). Then, you need to think about your logic, specifically what happens if you try to delete the last node of a linked list when there's only one node.

Upvotes: 3

pts
pts

Reputation: 87451

Initialize p:

void sterge_ultimul(element *lista) {
  element *p = lista, *q;
  while (p->urmator->urmator!=NULL)
    p=p->urmator;
  q=p->urmator;
  p->urmator=NULL;
  free(q);
}

Also I would add some error handling to check if the list is too short:

int sterge_ultimul(element *lista) {
  element *p = lista, *q;
  if (!p || !p->urmator) return 0;  /* error, list is too short */
  while (p->urmator->urmator!=NULL)
    p=p->urmator;
  q=p->urmator;
  p->urmator=NULL;
  free(q);
  return 1;
}

Upvotes: 2

Related Questions