Fenil
Fenil

Reputation: 396

Pointing to an uninitialised pointer vs Pointing to it after allocating memory for it

This doubt is very specific , consider the below code The two lines in the block, are the lines I am confused with. ie. when I exchange those two lines I get a segmentation fault , but this code runs.So my question is what is happening when I interchange the two lines?

#include<stdio.h>
#include<stdlib.h>
   typedef struct scale_node_s {
   char  note[4];
   struct scale_node_s *linkp;
 } scale_node_t;
 int main(){
 scale_node_t *scalep, *prevp, *newp,*walker; 
 int i;
 scalep = (scale_node_t *)malloc(sizeof (scale_node_t));
 scanf("%s", scalep->note);
 prevp = scalep;
 for(i = 0;  i < 7;  ++i) {
   //---------------------------------------
     newp = (scale_node_t *)malloc(sizeof (scale_node_t));      
     prevp->linkp = newp;
   //---------------------------------------
      scanf("%s", newp->note);
     prevp = newp;
  }
 walker = scalep;
  for(i = 0 ; i < 7 ; i++){
     printf("%s",walker->note);
     walker = walker->linkp; 
}


}

Upvotes: 0

Views: 41

Answers (2)

David C. Rankin
David C. Rankin

Reputation: 84579

In addition to the other answer, you have several additional issues that are making your list logic very confused and brittle. First, you are hardcoding loop iterations that may or may not match your input. The entire purpose of a linked-list is to provide a flexible data structure that allows you to store an unknown number of nodes. for(i = 0; i < 7; ++i) defeats that purpose entirely.

What is it you want to store? What is your input? (the note strings). Why not condition creation of additional nodes on input of a valid note? It can be as simple as:

    char tmp[MAXC] = "";
    ...
    while (scanf ("%3s", tmp) == 1) {
        ...   
        strcpy (newp->note, tmp);   /* set note and linkp */
        ...
    }

You are also leaving yourself wide open to processing invalid values throughout your code. Why? You fail to validate your user input and memory allocations. If either fail, you continue to blindly use undefined values from the point of failure forward. ALWAYS validate ALL user input and memory allocations. It is simple to do, e.g.

    if (!(scalep = malloc (sizeof *scalep))) {  /* allocate/validate */
        fprintf (stderr, "error: virtual memory exhausted, scalep.\n");
        return 1;
    }

    if (scanf ("%3s", scalep->note) != 1) {  /* validate ALL input */
        fprintf (stderr, "error: invalid input, scalep->note\n");
        return 1;
    }

Finally, there is no need for a prevp. That will only be required when removing or swapping nodes (you have to rewire the prev pointer to point to next (your linkp) after you remove or swap a node. You do neither in your code. There is also no need for an int value. You iterate current node = next node; to iterate though your list. (there are several variations of how to do this). Putting all the pieces together, and attempting to lay the code out in a slightly more logical way, you could do something similar to the following:

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

#define MAXC 4

typedef struct scale_node_s {
    char note[MAXC];
    struct scale_node_s *linkp;
} scale_node_t;

int main (void)
{
    scale_node_t *scalep, *newp, *walker;
    char tmp[MAXC] = "";

    if (!(scalep = malloc (sizeof *scalep))) {  /* allocate/validate */
        fprintf (stderr, "error: virtual memory exhausted, scalep.\n");
        return 1;
    }

    if (scanf ("%3s", scalep->note) != 1) {  /* validate ALL input */
        fprintf (stderr, "error: invalid input, scalep->note\n");
        return 1;
    }

    scalep->linkp = NULL;

    while (scanf ("%3s", tmp) == 1) {

        if (!(newp = malloc (sizeof *newp))) {  /* allocate/validate */
            fprintf (stderr, "error: virtual memory exhausted, newp.\n");
            break;
        }
        strcpy (newp->note, tmp);   /* set note and linkp */
        newp->linkp = NULL;

        walker = scalep;            /* set walker to scalep */
        while (walker->linkp)       /* find last node */
            walker = walker->linkp; /* linkp !NULL move to next node */

        walker->linkp = newp;
    }

    walker = scalep;    /* output list */
    while (walker) {
        printf ("%s\n", walker->note);
        walker = walker->linkp;
    }

    walker = scalep;    /* free list memory */
    while (walker) {
        scale_node_t *victim = walker;  /* save victim address */
        walker = walker->linkp;
        free (victim);                  /* free victim */
    }

    return 0;
}

Example Use/Output

$ echo "a b c d e" | ./bin/llhelp
a
b
c
d
e

Memory Use/Error Check

In any code you write that dynamically allocates memory, you have 2 responsibilities regarding any block of memory allocated: (1) always preserve a pointer to the starting address for the block of memory so, (2) it can be freed when it is no longer needed.

It is imperative that you use a memory error checking program to insure you do not attempt to write beyond/outside the bounds of your allocated block of memory, attempt to read or base a conditional jump on an uninitialized value, and finally, to confirm that you free all the memory you have allocated.

For Linux valgrind is the normal choice. There are similar memory checkers for every platform. They are all simple to use, just run your program through it.

$ echo "a b c d e" | valgrind ./bin/llhelp
==25758== Memcheck, a memory error detector
==25758== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==25758== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==25758== Command: ./bin/llhelp
==25758==
a
b
c
d
e
==25758==
==25758== HEAP SUMMARY:
==25758==     in use at exit: 0 bytes in 0 blocks
==25758==   total heap usage: 5 allocs, 5 frees, 80 bytes allocated
==25758==
==25758== All heap blocks were freed -- no leaks are possible
==25758==
==25758== For counts of detected and suppressed errors, rerun with: -v
==25758== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Always confirm that you have freed all memory you have allocated and that there are no memory errors.

Look things over and let me know if you have any additional questions.

Upvotes: 1

nvi9
nvi9

Reputation: 1893

The line newp = (scale_node_t *)malloc(sizeof (scale_node_t)); allocates a piece of memory needed to hold an instance of scale_node_t and makes newp to hold that address. On the next line, you pass newp to a struct to be the value of linkp.
Since on the first run of the loop newp is defined, but its value is not determined, it can hold several values depending on OS (and maybe on compiler too): memory waste, or 0 (so newp even can be null pointer there), segmentation fault occures hence.
It is not allowed to use any variable before initialization (pointers are actually variables, holding a memory address as a number), however some editor/environment/compiler may not warn you about it at compile time.

Upvotes: 1

Related Questions