0

I'm trying to implement a stack using a linked list in C but I am getting segfaults whenever I try to call the value after I push a new one onto the stack. I know this is happening because the program still says the stack is null even if I add to it. For some reason the changes that I make in the push don't stay when the function terminates and I can't figure out why.

Here's my code for the stack struct, initialization, and push:

typedef struct stack
{
    int value;
    struct stack * next;
} * stack_T;

stack_T
new_stack()
{
    return NULL;
}

int
push_stack(stack_T s, int data)
{
    stack_T new = malloc(sizeof(stack_T));
    new = s;
    if (s == NULL)
    {
        s = malloc(sizeof(stack_T));
        if (s == NULL)
            return 1;
    }
    s->value = data;
    s->next = new;
    return 0;
}

EDIT: Thanks for the help but I forgot to mention that the push has to be done with exactly those parameters, it's part of an assignment. I'm not looking for how to do it but rather what I'm doing wrong. I know that I can emulate pass by reference but as I said it has to be: 

int push_stack(stack_T s, int data)

I've made structs before with this style and had functions that just take them as a parameter and changes stay but they won't in this case and I have no idea why.

forev3r
  • 553
  • 1
  • 4
  • 9
  • 2
    You should generally not create type-aliases of pointers, like `stack_T`. That is because `malloc(sizeof(stack_T))` allocates space enough ***for the pointer***, not for the structure. – Some programmer dude Mar 31 '17 at 16:46
  • You should also take some time to search for and read about *emulating call by reference in c*. – Some programmer dude Mar 31 '17 at 16:47
  • 1
    You also malloc a stack, but then overwrite the only reference in the next line.... You should draw a picture of what's pointing to what, and walk through your code... There's a bunch of mistakes... – blackghost Mar 31 '17 at 16:51
  • I suggest avoiding "new" as a variable name. It's legal in C, but it's a reserved word in C++, and that can potentially make trouble for you later. – John Bollinger Mar 31 '17 at 17:12
  • More relevant to the question, I suggest you write *documentation* (i.e. in comments) for the type, the functions, and the function implementations. These should be sufficient to prove (in an informal sense) that your code is right. Since your code is in fact *wrong*, this process should not only help you discover what's wrong, but also should make it clear how to make it right. – John Bollinger Mar 31 '17 at 17:21
  • I've sadly been working on this problem for a few days now and the actual code inside the push has changed many times. Even if I do something like initialise the stack with some arbitrary value and have it point to NULL, and then use the push function to simply change that value, it doesn't stick outside the function and I have no idea why. I've tested inside the function and the value is there but when I check outside it's gone. – forev3r Mar 31 '17 at 17:26

1 Answers1

0

It looks like you haven't labeled s as a double pointer. It looks like you're attempting to push into the front of the linked list. What you need is to pass in double pointer of the reference to the head of the stack in order to change the address value stored at the memory. This way you would always have reference the top of the stack. 

int
push_stack(stack_T **s, int data)
{
    stack_T *new = malloc(sizeof(stack_T));
    if (new == NULL)
        return (1);
    if (s == NULL)
    {
        free(new);
        return (1);
    }
    new->value = data;
    new->next = *s;
    *s = new;
    return 0;
}

I would have to see how your main function is being called that would result in the segfault. However, it does seem that you don't have the proper reference to the top of the stack. Also, I don't quite understand why you are malloc out space for two nodes. Malloc returns a pointer that is being claimed by the size that you've designated.

what you wrote here:

stack_T new = malloc(sizeof(stack_T));
new = s;
if (s == NULL)
    {
        s = malloc(sizeof(stack_T));
        if (s == NULL)
            return 1;
    }

Will never be met with NULL is a a type of void pointer and s as you assigned it is not of a pointer type, so you are attempting to store a variable of the 12 bytes when malloc returns a pointer of 8 bytes.

Also a tip is that you can compile with gcc -g and use valgrind to determine the line in which the segfault is occuring

richard_d_sim
  • 793
  • 2
  • 10
  • 23
  • Nope, you've missed that `stack_T` is already a pointer type, so `stack_T *` is indeed a double pointer. Rude of the OP to hide that behind a typedef, really. – John Bollinger Mar 31 '17 at 17:06
  • S is initialised as NULL so it can be NULL in the first check and the reason for the second check is in the (rare) case that malloc can not find memory for whatever reason it will return NULL. Also as John said, stack_T is a pointer already. Thank you for the help though. – forev3r Mar 31 '17 at 17:20