[3rd update] I made some changes to David's code. I changed: n->next = s->top;s->top = n; to n->next = NULL;s->top = n; like H.S. suggested; and also added different features to the main function. I'm trying to get the char name and int value as input from the user while the program is running. The program should keep taking data from the user, storing it in the stack and allocating memory accordingly. The program only ends when the user types 'end' (char) or 0 (int). The code gets compiled, but that's it, after that, any input and the program stops. Also, am I wasting memory using fgets(str,20,stdin) ? What if I don't use all that size. Any feedback is very much appreciated! ///PROGRAM STRUCTURE #include <stdio.h> #include <stdlib.h> #include <string.h> struct stack_node{ int value; char *name; struct stack_node * next; }; typedef struct stack_node stack_node; struct stack{ stack_node *top; int size; }; typedef struct stack stack; stack *create_stack() { stack *s = malloc (sizeof *s); if (!s) { perror ("malloc-s"); return NULL; } s->size = 0; s->top = NULL; return s; } int empty_stack (stack * s) { return s->size == 0; } stack_node *push_stack (stack *s, int value, char *name) /// **** { stack_node *n = malloc (sizeof *n); if (!n) { perror ("malloc-n"); return NULL; } n->value = value; n->name = malloc (strlen (name) + 1); if (!n->name) { perror ("malloc-name"); return NULL; } strcpy (n->name, name); n->next = NULL; /// **** s->top = n; s->size++; return n; } stack_node *pop_stack (stack *s) { if (s->size > 0) { stack_node *node = s->top; s->top = s->top->next; s->size--; return node; } return NULL; } void free_node (stack_node *n) { if (n->name) free (n->name); free (n); } void free_stack (stack *s) { while (s->size > 0) { stack_node *victim = s->top; s->top = s->top->next; free_node (victim); s->size--; } free (s); } int main (void) { stack *s = create_stack(); stack_node *node = NULL; char *str; ///NAME int vs; ///VALUE int i=0; if (!s) return 1; do{ printf("NAME{%d]: ",i); fgets(str,20,stdin); ///***? printf("VALUE[%d]: ",i); scanf(" %d",&vs); if (push_stack (s, vs, str) == NULL) { fprintf (stderr, "error: push node failed '%d'.\n", i); return 1; } i++; }while(str != 'end' || vs != 0); i=0; while ((node = pop_stack(s)) != NULL) { printf ("value[%d]: %d name[%d]: %s\n", i,node->value, i,node->name); i++; free_node (node); } free_stack (s); return 0; } ///********** do{ printf("NAME[%d]: ",i); if(fgets(buf,sizeof buf,stdin)){ int rtn = sscanf (buf, "%63[^\n]", name); if(rtn == 1){ printf("VALUE[%d]: ",i); scanf(" %d",&vs); } } push_stack(s,vs,name); if (s == NULL) { fprintf (stderr, "error: push node failed '%d'.\n", i); return 1; } i++; }while(node->name != "end" || node->value != 0);

stacks with strings and integers - C

Answers (3)

Reputation: 84607

Well.. It's obvious you have put effort into your code, but it is also obvious you are still struggling to wrap your head around storing a struct as a node within the stack (and with basic string handling issues).

First, there is no need to cast the return of malloc, see See: Do I cast the result of malloc?. Next, always size your allocation for an object based on the derefernced pointer to the object. For example stack *s declares s as a pointer to type stack. If s is a pointer to stack, then *s is type stack. Sizing your allocations with the actual pointer itself with eliminate any chance of an incorrect size. Further, it is your responsibility to validate every allocation by checking the return is not NULL. With that, your create_stack() could be:

stack *create_stack()
{
    stack *s = malloc (sizeof *s);  /* size from dereferenced pointer */
    if (!s) {                       /* validate every allocation */
        perror ("malloc-s");
        return NULL;
    }

    s->size = 0;
    s->top = NULL;

    return s;
}

Your empty_stack is unused, but otherwise OK.

Your push_stack function is where a number of problems lie. First you cannot allocate for the node and the string at the same time! (even if you could, you would be one-byte too small as you have forgotten space for the nul-terminating character at the end of the string). You declare:

struct stack_node{
    int value;
    char *name;
    struct stack_node * next;
};

You must allocate first for your node, then allocate storage for your pointer name -- remembering to validate the allocation as done in create_stack(), e.g.

    stack_node *n = malloc (sizeof *n); /* allocate node only */
    if (!n) {                           /* validate every allocation */
        perror ("malloc-n");
        return NULL;
    }

    n->value = value;
    n->name = malloc (strlen (name) + 1);   /* allocate strlen + 1 chars */
    if (!n->name) {                         /* validate! */
        perror ("malloc-name");
        return NULL;
    }

Next, you cannot assign strings in C (aside from initialization of a pointer to point to a string-literal, e.g. char *name = "foo";) In C, you must copy your string to the valid storage your create for n->name. For example:

    strcpy (n->name, name);     /* you cannot assign strings, use strcpy */

Further, how are you going to indicate to the caller, whether your allocations succeeded or failed if you declare push_stack as type void??

Always select a return type that can indicate success/failure where required. With that in mind, you can simply return a pointer to the new node (which is convenient for immediate use anyway), or return NULL on failure, e.g.

/* choose return type that can indicate success/failure */
stack_node *push_stack (stack *s, int value, const char *name)
{
    stack_node *n = malloc (sizeof *n); /* allocate node only */
    if (!n) {                           /* validate every allocation */
        perror ("malloc-n");
        return NULL;
    }

    n->value = value;
    n->name = malloc (strlen (name) + 1);   /* allocate strlen + 1 chars */
    if (!n->name) {                         /* validate! */
        perror ("malloc-name");
        return NULL;
    }
    strcpy (n->name, name);     /* you cannot assign strings, use strcpy */
    n->next = s->top;
    s->top = n;
    s->size++;

    return n;
}

You do not want two-separate pop function. Instead of pop_stack_value and pop_stack_name, you simply want a pop_stack that returns a pointer to the top node. You can then access either the value or name (e.g. node->value or node->name) as required.

It also makes sense to declare a couple of simple helper functions to free the memory allocated when it is no longer needed. You are dealing with nodes and the stack, so a free_node helper and a free_stack helper make sense. (and remember, you may want to free the stack before it is empty, so code the free_stack accordingly), e.g.

void free_node (stack_node *n)  /* free node helper function */
{
    if (n->name)
        free (n->name);

    free (n);
}

void free_stack (stack *s)      /* free stack helper (need not be empty) */
{
    while (s->size > 0) {
        stack_node *victim = s->top;
        s->top = s->top->next;
        free_node (victim);
        s->size--;
    }

    free (s);
}

Putting all the pieces together in a short example, you could do something similar to the following:

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

struct stack_node{
    int value;
    char *name;
    struct stack_node * next;
};
typedef struct stack_node stack_node;

struct stack{
    stack_node *top;
    int size;
};
typedef struct stack stack;

stack *create_stack()
{
    stack *s = malloc (sizeof *s);  /* size from dereferenced pointer */
    if (!s) {                       /* validate every allocation */
        perror ("malloc-s");
        return NULL;
    }

    s->size = 0;
    s->top = NULL;

    return s;
}

int empty_stack (stack * s) {       /* s->size is type 'int' */
    return s->size == 0;
}

/* choose return type that can indicate success/failure */
stack_node *push_stack (stack *s, int value, const char *name)
{
    stack_node *n = malloc (sizeof *n); /* allocate node only */
    if (!n) {                           /* validate every allocation */
        perror ("malloc-n");
        return NULL;
    }

    n->value = value;
    n->name = malloc (strlen (name) + 1);   /* allocate strlen + 1 chars */
    if (!n->name) {                         /* validate! */
        perror ("malloc-name");
        return NULL;
    }
    strcpy (n->name, name);     /* you cannot assign strings, use strcpy */
    n->next = s->top;
    s->top = n;
    s->size++;

    return n;
}

stack_node *pop_stack (stack *s)    /* return the node on pop */
{
    if (s->size > 0) {
        stack_node *node = s->top;
        s->top = s->top->next;
        s->size--;
        return node;    /* caller is responsible for freeing node */
    }

    return NULL;
}

void free_node (stack_node *n)  /* free node helper function */
{
    if (n->name)
        free (n->name);

    free (n);
}

void free_stack (stack *s)      /* free stack helper (need not be empty) */
{
    while (s->size > 0) {
        stack_node *victim = s->top;
        s->top = s->top->next;
        free_node (victim);
        s->size--;
    }

    free (s);
}

int main (void) {

    const char *str[] = { "john", "jack", "jill", "sally", "sue" };
    int n = sizeof str/sizeof *str;
    stack *s = create_stack();
    stack_node *node = NULL;

    if (!s)         /* validate !!! */
        return 1;

    for (int i = 0; i < n; i++)
        if (push_stack (s, i+1, str[i]) == NULL) {
            fprintf (stderr, "error: push node failed '%d'.\n", i);
            return 1;
        }

    while ((node = pop_stack(s)) != NULL) {
        printf ("value:  %2d    name: %s\n", node->value, node->name);
        free_node (node);   /* free node */
    }
    free_stack (s);     /* free stack */

    return 0;
}

Example Use/Output

$ ./bin/stack_struct
value:   5    name: sue
value:   4    name: sally
value:   3    name: jill
value:   2    name: jack
value:   1    name: john

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 access memory or write beyond/outside the bounds of your allocated block, 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.

$ valgrind ./bin/stack_struct
==4204== Memcheck, a memory error detector
==4204== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==4204== Using Valgrind-3.12.0 and LibVEX; rerun with -h for copyright info
==4204== Command: ./bin/stack_struct
==4204==
value:   5    name: sue
value:   4    name: sally
value:   3    name: jill
value:   2    name: jack
value:   1    name: john
==4204==
==4204== HEAP SUMMARY:
==4204==     in use at exit: 0 bytes in 0 blocks
==4204==   total heap usage: 11 allocs, 11 frees, 161 bytes allocated
==4204==
==4204== All heap blocks were freed -- no leaks are possible
==4204==
==4204== For counts of detected and suppressed errors, rerun with: -v
==4204== 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.

Finally, to avoid most of these problems to begin with, always compile with warnings enabled, and do not accept code until it compiles cleanly without warning. To enable warnings add -Wall -Wextra to your gcc or clang compile string. (add -pedantic for several additional warnings). For clang, instead you can use -Weverything. For VS (cl.exe on windoze), add /W3 (or use /Wall but you will get quite a few extraneous windows non-code related warnings). Read and understand each warning. They will identify any problems, and the exact line on which they occur.

You can learn as much about coding by simply listening to what your compiler is telling you as you can from most tutorials.

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

Taking value Input, then name

When you take user input, consider using fgets for reading both the name and value (and then use sscanf (or strtol) to convert to int). You avoid many pitfalls with scanf and failure to empty stdin after a matching failure (which will generally lead to an infinite loop as well as Undefined Behavior)

You purposely want to loop continually until you validate your have the input you ask for -- and that it meets your requirements (range, etc...). You also must protect against the user canceling input by generating a manual EOF (e.g. Ctrl+d on Linux or Ctrl+z on windoze, but see: CTRL+Z does not generate EOF in Windows 10)

/* if you need constants, #define one (or more), or use a global enum */
#define MAXC    1024    /* max chars for input buffer (don't skimp) */
#define MAXNM     64    /* max length for name buffer (ditto) */
...
int main (void) {

    stack *s = create_stack();
    stack_node *node = NULL;

    if (!s)         /* validate !!! */
        return 1;

    for (;;) {      /* loop continually taking input until exit conditon */
        char buf[MAXC]  = "",   /* line buffer for fgets */
            name[MAXNM] = "";   /* buffer for parsing name from line */
        int value = 0;          /* int to parse from line */

        for (;;) {  /* loop continually until you get valid int */
            fputs ("\nenter value: ", stdout);  /* prompt for value */
            if (fgets (buf, MAXC, stdin)) {     /* validate line read */
                if (sscanf (buf, "%d", &value) == 1) { /* convert to int */
                    if (value == 0) {   /* your exit condition */
                        fputs ("  value == 0 (input done)\n", stderr );
                        goto inputdone; /* jump out of both loops */
                    }
                    break;
                }
                else    /* non-integer (non-numeric) input */
                    fputs ("  error: invalid integer input.\n", stderr);
            }
            else {  /* fgets returned EOF (user canceled) */
                fputs ("(user canceled input)\n", stderr);
                goto inputdone; /* jump out of both loops */
            }
        }

        for (;;) {      /* loop continually until name recieved */
            fputs ("enter name : ", stdout);        /* prompt name */
            if (fgets (name, MAXNM, stdin)) {       /* read name */
                size_t len = strlen (name);         /* get length */
                if (len && name[len-1] == '\n') {   /* last char \n ? */
                    name[--len] = 0;                /* overwrite with \0 */
                    if (len)                    /* check 1 char remains */
                        break;                  /* got good name, go push */
                    else    /* user just pressed [Enter] */
                        fputs ("  error: empty-line.\n", stderr);
                }
                else if (len == MAXNM -1)   /* was input too long? */
                    fputs ("  warning: name truncated.\n", stderr);
            }
            else {  /* fgets returned EOF (user canceled) */
                fputs ("(user canceled input)\n", stderr);
                goto inputdone; /* jump out of both loops */
            }
        }

        push_stack (s, value, name);    /* push value and name on stack */
    }
    inputdone:;

    puts ("\nvalues stored in stack\n");
    while ((node = pop_stack(s)) != NULL) {
        printf ("value:  %2d    name: %s\n", node->value, node->name);
        free_node (node);   /* free node */
    }
    free_stack (s);     /* free stack */

    return 0;
}

(note: it would probably be better to end input when the user presses Enter on an empty line rather than looking for a value == 0 (because an int can be zero))

Example Use/Output

$ ./bin/stack_struct_input_simple

enter value: 1
enter name : Frank Zappa

enter value: 2
enter name :
error: empty-line.
enter name : Jimmy Buffet

enter value: 3
enter name : Grahm Nash

enter value: 4
enter name : John Bonham

enter value: 0
value == 0 (input done)

values stored in stack

value:   4    name: John Bonham
value:   3    name: Grahm Nash
value:   2    name: Jimmy Buffet
value:   1    name: Frank Zappa

See if you can go though the input routine above and understand why it is written the way it is. (I also have one that will accept both value name or value then name) You can be as flexible as you want with input as long as you follow the rules and validate everything, protect against a manual EOF and always know what remains in your input buffer (e.g. stdin).

Upvotes: 1

H.S.

Reputation: 12679

Few of problems in your code:

Problem 1:
Look at these statement of function push_stack():

    n->next = s->top;
    s->top = n;

The next of stack node n is pointing to top of stack and top of stack is pointing back to node n. The next of the node created should be NULL as it will be top node of stack. You don't need this statement n->next = s->top;. It should be:

    n->next = NULL;
    s->top = n;

Problem 2:

In main(), you are doing:

    printf("STACK: %d \n",pop_stack_value(s));
    printf("STACK: %s \n", pop_stack_name(s));

and in function pop_stack_value(), you are doing:

    stack_node * sn = s->top;
    s->top = s->top->next;
    free(sn);

s->top is pointing to the top of stack and the top of stack is pointing back to node (as you did in push_stack()). The free(sn) will free the top of stack. Now in function pop_stack_name(), you are doing

    char * name = s->top->name;
    stack_node * sn = s->top;
    s->top = s->top->next;
    s->size--;
    free(sn);

s->top is pointing to memory which has been already freed in pop_stack_value() and you are trying to access memory which your program doesn't own. This is undefined behavior.

The way you are doing pop operation is completely wrong. In the pop operation of stack, you should pop the whole node instead of popping individual value of node. Once the node is popped, set the top of stack to point to the previous node. For that you need to traverse the whole list because you are using singly linked list. As an alternative, you can have doubly linked list in which every node is having its previous node address. With this resetting top of stack would not be an expensive operation.

Upvotes: 0

dgnuff

Reputation: 3557

Is there any reason not to alter your stack_node struct to directly include the name.

struct stack_node{
    int value;
    struct stack_node * next;
    char name[1];
};

This particular form has the advantage that if you call malloc as follows:

struct stack_node * new_node = malloc(sizeof *new_node + strlen(name));

you get exactly the correct amount of memory, including space for the trailing NUL byte on the string.

Ob warning. Skip the last paragraph if you are humor impaired.

Still here? Good. Failing all this, you could always try a Soap interface. But then perhaps we should never talk about that. ;)

Upvotes: 0

stacks with strings and integers - C

Answers (3)

Related Questions