Error while printing chained list

Question

Hello, I have an error in the function that prints a chained list.

I think the error is in the print function’s for, but I may also be saving the address of prox wrongly.

Follows the code:

/* 
 * File:   main.c
 * Author: pmargreff
 *
 * Created on 1 de Dezembro de 2014, 20:17
 */

/*
 * cria um struct que contém um nodo do tipo inteiro
 * e um ponteiro para uma celula do tipo do próprio struct
 */
struct cel {
    int nodo;
    struct cel *prox;
};
typedef struct cel celula;

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

celula *cria(void); //por que a função é um ponteiro ?
void insere(int x, celula *pont);
void imprime(celula *inicio);
/*
 * 
 */
int main(int argc, char** argv) {
    int i;
    celula *inicio;
    inicio = cria();

    for (i = 0; i < 10; i++) {
        insere(i, inicio);
    }
    imprime(inicio);

    return (EXIT_SUCCESS);
}

/*
 * cria uma função que recebe uma célula apontando para o ínicio 
 * e que aloca espaço para um tipo célula e passa a ser apontada pela 
 * célula inicio
 */
celula *cria(void) {
    celula *primeira;
    primeira = malloc(sizeof (struct cel));
    primeira->prox = NULL;
    return primeira; //perguntar para o andŕe o que isso significa
}

/*
 * cria uma nova váriavel do tipo célula chamada de "nova"
 * aloca espaço para ela, insere o valor no campo do seu conteúdo
 * o ponteiro da célula (ou seja) prox, recebe o valor que o ponteiro 
 * da célula anterior a ela tinha, o ponteiro da primeira aponta para 
 * nova
 */
void insere(int x, celula *head) {
    celula *nova;
    nova = malloc(sizeof (struct cel));
    nova -> nodo = x;
    nova -> prox = head -> prox;
    head -> prox = head;
}

void imprime(celula *inicio){
    celula *pont;
    for (pont = inicio -> prox; pont != NULL; pont = pont->prox)
        printf ("   %d\n", pont->nodo);
}

After a few more attempts, I have a new function used for insertion.

void *insere(int x, celula *head) {
    celula *nova;
    nova = malloc(sizeof (struct cel));
    nova -> nodo = x;
    nova -> prox = head -> prox;
    head -> prox = nova;
}

Answer 1

First of all:

void imprime(celula *inicio){
    celula *pont;
    for (pont = inicio -> prox; pont != NULL; pont = pont->prox)
        printf ("   %d\n", pont->nodo);
}

The right thing would be pont = inicio; and not pont = inicio -> prox. The reason is that the inicio is the first element. Otherwise you would start from the second, and the program would crash if you received a list of 1 or 0 elements.

There are more problems too:

void insere(int x, celula *head) {
    celula *nova;
    nova = malloc(sizeof (struct cel));
    nova -> nodo = x;
    nova -> prox = head -> prox;
    head -> prox = head;
}

Was the idea to insert at the beginning or end? Whatever is wrong for both. The line head -> prox = head; makes the cell point to itself as the next element, causing a list with a loop. Also, there is a memory leak, because from the head it is not possible to arrive at nova which was allocated but lost in inaccessible memory.

To insert at the beginning:

celula *insere_no_inicio(int x, celula *head) {
    celula *nova;
    nova = malloc(sizeof (struct cel));
    nova -> nodo = x;
    nova -> prox = head;
    return nova;
}

To insert at the end:

celula *insere_no_final(int x, celula *head) {
    celula *nova;
    nova = malloc(sizeof (struct cel));
    nova -> nodo = x;
    if (head == NULL) {
        nova -> prox = NULL;
        return nova;
    }
    celula *ultima = head;
    while (ultima->prox != NULL) {
        ultima = ultima->prox;
    }
    ultima -> prox = nova;
    return head;
}

The idea of creating the list is to create the first element of it. This is what is returned by the function cria.

I would particularly modify the function cria to already fill the element of the created node:

celula *cria(int x) {
    celula *primeira;
    primeira = malloc(sizeof (struct cel));
    primeira->prox = NULL;
    primeira->nodo = x;
    return primeira;
}

Or else, just use the function insere_no_inicio passing as the second parameter NULL, in this case the function cria can be either totally eliminated, or reduced to that:

celula *cria(int x) {
    return insere_no_inicio(x, NULL);
}

EDITED:

Finally, to use these new creation functions:

celula *inicio = NULL;
for (i = 0; i < 10; i++) {
    inicio = insere_no_inicio(i, inicio);
}

// OU:

celula *inicio = NULL;
for (i = 0; i < 10; i++) {
    inicio = insere_no_final(i, inicio);
}

And you can’t forget to destroy the list after you finish working with her:

void destroi(celula *inicio) {
    celula *proximo = inicio;
    while (proximo != NULL) {
        celula *destruir = proximo;
        proximo = proximo -> prox;
        free(destruir);
    }
}

You suggested this new role:

void *insere(int x, celula *head) {
    celula *nova;
    nova = malloc(sizeof (struct cel));
    nova -> nodo = x;
    nova -> prox = head -> prox;
    head -> prox = nova;
}

Its new function is wrong, because if it receives a list [A, B, C], it changes the pointers making it [A, N, B, C], where N is the new cell. That is, it always enters in the second position. If you receive an empty list, it will crash.

Maybe it seems to work because its function cria started the list with an element without value, and only from the second element did the values come. But in this case you will always be wasting the first element and whenever you want to use your list for anything, you should start with the second.

At last, the return void *, that is not the same thing as void, makes no sense. Use void * means to say that the function returns an undetermined type of pointer, but in fact it returns nothing.