Lista Bidireccional "Violación de segmentación"

[Areees777]

Hola,

Mi problema es el siguiente:
He realizado una lista bidireccional y a la hora de insertar un dato dentro de ella, me da un error de segmentación. He utilizado gdb para ver donde se encuentra este error, y me dice lo siguiente:

Program received signal SIGSEGV, Segmentation fault.
0x000000000040074d in insertar_detras (l=0x7fffffffe230, element=3)
    at Listadin.c:55
55         temporal->next->prev=temporal;

_____________________________________________________________________

Os dejo aquí el código de la lista, a ver si me podéis decir donde está el error, que no encuentro..

#include <stdio.h>
#include <stdlib.h>

typedef struct _nodo{
   int element;
   struct _nodo *next;
   struct _nodo *prev;
}nodo;

typedef struct{
   nodo *first;
   nodo *last;
   nodo *pdi;
}lista;

void crear_lista(lista *l){
   l->first=(nodo *)malloc(sizeof(nodo));
   if (l->first == NULL){
      printf("Error!\n");
   }else{
      l->last=(nodo *)malloc(sizeof(nodo));
      if (l->last == NULL){
         printf("Error!\n");
      }else{
         l->first->next=l->last;
         l->last->prev=l->first;
         l->first->prev=NULL;
         l->last->next=NULL;
         l->pdi=l->last;
      }
   }

}

void insertar_detras(lista *l, int element){
   nodo *temporal;
   temporal=(nodo *)malloc(sizeof(nodo));
   if (temporal == NULL){
      printf("Error!\n");

   }else{
      temporal->element=element; //1. Asigna el elemento nuevo al temporal (nodo nuevo que hemos creado)
      temporal->next=l->pdi->next; //2. La casilla "next" del nodo temporal apunta al siguiente de la lista
      l->pdi->next=temporal;
      temporal->next->prev=temporal;
   }
}

int main(void) {
   int salir=0, opcion;
         int element;
         int pos;
         lista l;



           while(salir==0){
                           printf("-------Lista dinámica ---------\n");
                           printf("1. Crear Lista\n");
                           printf("2. Insertar datos Detras\n");
                           printf("3. Insertar datos delante\n");
                           printf("4. Consulta un elemento\n");
                           printf("5. Exit\n");

                           printf("Escoja una opcion:\n");
                           scanf("%d",&opcion);
                           printf("La opcion que ha escogido es:\n",opcion);
                           switch(opcion)
                           {
                                         case 1:

                                             crear_lista(&l);
                                             printf("Lista creada\n");
                                              break;

                                         case 2:
                                             printf("Numero a introducir:\n");
                                             scanf("%d",&element);
                                             insertar_detras(&l,element);
                                              break;

                                         case 3:

                                            break;

                                         case 4:

                                            break;


                                         case 5:
                                             salir=1;
                                              printf("EXIT\n");
                                              break;
                                         default:
                                              printf("La opcion introducida no existe\n");
                                              printf("Vuelva a intentarlo\n");
                           }
                           }

Muchas Gracias, y espero que me podáis echar una mano.

Un Saludo!

[eferion]

Sugerencias:

* formatea el texto utilizando las etiquetas que están en el combo GeSHi... elige la opción c o c++ según proceda.

* en el caso de que una parte importante de los campos de una estructura recién creada con new deban ser null, es mejor usar calloc en vez de malloc... calloc inicializa la memoria a cero, con lo que hace parte del trabajo por tí.

* las indirecciones son peligrosas, si te las puedes ahorrar mejor. Me refiero por ejemplo al código siguiente:

Código (c) [Seleccionar] Expandir


void crear_lista(lista *l){
   l->first=(nodo *)malloc(sizeof(nodo));
   if (l->first == NULL){
      printf("Error!\n");
   }else{
      l->last=(nodo *)malloc(sizeof(nodo));
      if (l->last == NULL){
         printf("Error!\n");
      }else{
         l->first->next=l->last;
         l->last->prev=l->first;
         l->first->prev=NULL;
         l->last->next=NULL;
         l->pdi=l->last;
      }
   }

}

l->first->next ... al final este código acaba siendo poco legible y es fácil confundirse.

Queda más limpio si creas primero los nodos usando un puntero propio de nodo... le asignas los valores que procedan y después asocias los nodos a la lista:

Código (c) [Seleccionar] Expandir


void crear_lista(lista *l){
   nodo* first = (nodo*) calloc( 1, sizeof(nodo) );
   nodo* last = (nodo*) calloc(1, sizeof(nodo) );

   if ( first == NULL || last == NULL )
   {
     free( first ); // evitamos lagunas de memoria, tu codigo no controla esto
     free( last ); // evitamos lagunas de memoria, tu codigo no controla esto
     printf( "Error!\n" );
   }
   else
   {
     first->next = last;
     last->prev = first;

     l->first = first;
     l->last = last;
   }
}

Además imagino que la idea es que cada nodo tenga valor... si creas una lista de cero lo normal es que tenga un solo elemento, no dos... además de que deberías poder asignarle un valor a ese elemento:

Código (c) [Seleccionar] Expandir


void crear_lista(lista *l, int valor){
   nodo* first = (nodo*) calloc( 1, sizeof(nodo) );

   if ( first == NULL )
   {
     printf( "Error!\n" );
   }
   else
   {
     nodo->element = valor;
     l->first = first;
     l->last = first;
     l->pdi = first;
   }
}

* La variable pdi no entiendo muy bien cual es su uso... además me parece que sea cual sea su uso puede estar provocando errores:

Código (c) [Seleccionar] Expandir


void insertar_detras(lista *l, int element){
  // ...
   }else{
      temporal->element=element; //1. Asigna el elemento nuevo al temporal (nodo nuevo que hemos creado)
      temporal->next=l->pdi->next; //2. La casilla "next" del nodo temporal apunta al siguiente de la lista
      l->pdi->next=temporal;
      temporal->next->prev=temporal;
   }
}

En ese código no estás actualizando pdi, por ende siempre va a apuntar al primer elmento original, aunque deje de serlo en el futuro... la segunda vez que llames a esta función y sucesivas vas a estar creando una configuración un tanto extraña en tu lista...

Realmente yo creo que pdi no es necesario en absoluto. Tendría sentido si tuvieses que dejar seleccionado un nodo para que las operaciones de insertar delante e insertar detrás se aplicasen sobre el nodo seleccionado... pero para eso tendrías que tener una opción en el menú para seleccionar un elemento que a no ser que sea la 4ª... Voy a presuponer que es realmente así

Código (c) [Seleccionar] Expandir


void insertar_detras(lista *l, int element){
   nodo *temporal;
   temporal=(nodo *)malloc(sizeof(nodo));
   if (temporal == NULL)
   {
      printf("Error!\n");
   }else{
      nodo* previo = l->pdi->prev;
      nodo* siguiente = l->pdi;

      temporal->element=element; //1. Asigna el elemento nuevo al temporal (nodo nuevo que hemos creado)
      temporal->next=siguiente; //ERROR: no es pdi->next, es pdi, ya que vas a insertarlo detras de PDI, no delante
      temporal->prev=previo; // ERROR: no has hecho esta asignacion... luego la lista ya no va a tener valores consistentes

      if ( previo != NULL ) // Puede que el nuevo elemento pase a ser el primero de la lista
        previo->next = temporal; // Otra asignacion no realizada... mas inconsistencias en la lista.

      siguiente->prev = temporal; // Esta asignacion si la has hecho bien
   }
}

Lo siguiente, además, merece una atención aparte:

Código (c) [Seleccionar] Expandir


      temporal->next=l->pdi->next; //2. La casilla "next" del nodo temporal apunta al siguiente de la lista
      l->pdi->next=temporal;
      temporal->next->prev=temporal;


Pasos seguidos:

* temporal->next = pdi->next; --> Mal vamos si temporal tiene que ir antes de pdi lo lógico sería temporal->next = pdi
* l->pdi->next = temporal; --> Seguimos poniendo temporal después de pdi en vez de antes
* temporal->next->prev = temporal; --> O_o no esta mal del todo pero entenderás que la línea es fea de narices.
* YA?? y temporal->prev ?? no debería apuntar a l->pdi ?? aún así insisto en que con esto temporal acaba delante de pdi, no detrás.

Además como al crear los nodos nuevos no estás inicializando la memoria, si te dejas sin asociar algún puntero no te das cuenta porque tendrá valores, aunque sean extraños... con calloc esto no te pasaría.

* Realmente podrías unificar el proceso de inserción, de tal forma que añadir delante y añadir detrás podrían llamar a la misma función... ahorrarías código y quedaría un programa más limpio... pero eso ya te lo dejo a ti.

[rir3760]

Otros dos comentarios:

* En C no es necesario ni se recomienda la conversión explicita al llamar a las funciones calloc, malloc o realloc (para el caso cualquiera que retorne un valor de tipo "void *") ya que esa conversión es automática.

* El programa revienta después de crear la lista cuando se trata de agregar un nodo con la función "insertar_detras".

1) Después de crear la lista el valor de los campos last y pdi de la variable *l es el mismo, apuntan al mismo objeto (ultimo nodo en la lista).

2) Las lineas que causan el "Segmentation fault" son dos:

Código (c) [Seleccionar] Expandir

temporal->next = l->pdi->next;
/*
** l->pdi y l->last apuntan al mismo nodo, el ultimo. Su campo
** "next" es igual a NULL por razones obvias. El punto clave aqui
** es: temporal->next es NULL
*/

l->pdi->next = temporal; /* No impacta */

temporal->next->prev = temporal;
/*
** temporal->next->prev ==> NULL->prev ==> el error que mencionas
*/

Un saludo