2

I'm trying to implement a linked list, with a addToFront function to start with. Where here I'm just adding the number 5, to the front of the list. I know that if the list is empty, the list pointer should be Null, however, this does not seem to be the case.

EDITED FILES: I've edited the files( thanks to taskinoor's answer), which now provide the output of

0 5

Instead of

5

I have the header file:

#pragma once
#define _CRT_SECURE_NO_WARNINGS
#include <stdio.h>
#include <stdlib.h>
#include <conio.h>
#include <string.h>

typedef struct List {
    struct list * next;
    int value;
    int size;
}list;

void addToFront(int num, list **l);

void printList(list * l);

int getSize(list * l);

void initialize(list * l);

void freeList(list *l);

A c file "main.c"

#include "Header.h"

int main() {

    list l;

    initialize(&l);

    addToFront(5, &l);
    printList(&l);


    _getch();
    freeList(&l);
    return 0;
}

void printList(list * l) {
    list *current = l;
    while (current != NULL) {
        printf("%d ", current->value);
        current = current->next;
    }
}

void freeList(list *l) {
    list *current = l;
    while (current != NULL) {
        list *tmp = current;
        current = current->next;
        free(tmp);
    }
}

And an interface c file (which is incomplete)

#include "Header.h"

int getSize(list * l) {
    return l->size;
}

void initialize(list * l) {
    l->next = NULL;
    l->value = 0;
    l->size = 0;
}

// need to pass **l to update it
void addToFront(int num, list **l) {
    // allocate memory for new node
    list *tmp = (list *)malloc(sizeof(list));

    tmp->value = num;

    // new node should point to whatever head is currently pointing
    // even if head is NULL at beginning
    tmp->next = *l;

    // finally l needs to point to new node
    // thus new node becomes the first node
    *l = tmp;
}

However, when the addToFront function is called, the if statement is never executed. Which doesn't make sense, if the list is empty, shouldn't the list pointer be null?

Next I tried to manually set l == NULL in the Initialize function, but that didn't do anything either. Also, my print function loops infinity, which I presume is an issue with malloc. Any help would be greatly appreciated.

TTEd
  • 309
  • 1
  • 10
  • if `if (l == NULL)` then you __cannot__ dereference it as in `l->value= ...`!! – Paul Ogilvie Dec 19 '15 at 17:29
  • Hmm, but then how would I set the value for the new node to be inserted(which happens to be the first node in the list). Nonetheless, the bigger issue is why the if statement is not executing. – TTEd Dec 19 '15 at 17:30
  • ...and "manually `set l == NULL`" won't work too as `==` is the comparisson operator, not the assignment operator. Maybe you want to check if `l->next == NULL`? – Paul Ogilvie Dec 19 '15 at 17:31
  • But isn't 'l->next' the second node in the list, not the first? – TTEd Dec 19 '15 at 17:31
  • The `if` statement is probably executed but `l` is not `NULL` as you have allocated memory for it in main. `l->next` is NULL though – Paul Ogilvie Dec 19 '15 at 17:32
  • I tried not allocating memory, instead using the addresses "&" as parameters to functions, the same thing happens. I.e instead of doing **list *l**, I did **list l**. – TTEd Dec 19 '15 at 17:33
  • struct (and class) names should be capitalized by convention – gen-y-s Dec 20 '15 at 14:07

4 Answers4

1

Ok, lets begin from the last part:

I tried to manually set l == NULL in the Initialize function, but that didn't do anything either

Actually it did something, but once you returned from the initialize function, that change was lost. Here's why:

When you say initialize(l), in the initialize function body you get a copy of the original pointer l. Then you make that pointer, point to NULL. When that function returns the original pointer l still points to the initial memory (the one allocated with malloc). If you do want this initialize function to have such behavior, you should change it to:

initalize(list **l)

Regarding the addToFront() function, actually if it was executed, you would get a segfault! You check:

if (l == null)

and if it is, you try to dereference the NULL pointer!

l->value = num;
l->next = NULL;
l->size++;

Lastly, in your print fucntion you do not advance your pointer. You should write something like

l=l->next

in order to work

sestus
  • 1,875
  • 2
  • 16
  • 16
  • `initalize(list **l)` is wrong and/or not necessary. `l` is a pointer. Initialize only sets the members to zero. – Paul Ogilvie Dec 19 '15 at 17:33
  • Not necessary I agree. Why wrong? I said **if you want such behavior** (modify the list pointer inside the function) do that. @PaulOgilvie what is your proposed way of changing a pointer inside a function? – sestus Dec 19 '15 at 17:34
  • The point is that initialize, as written by OP, does not modify the pointer. Hence ** is not needed. – Paul Ogilvie Dec 19 '15 at 17:39
  • Do you see the quote from question in the beggining of my answer? That says that he tried to do exactly that -- change the l pointer in the initialize function. But I guess you already saw that, you just needed to comment / vote down. I get it – sestus Dec 19 '15 at 17:42
  • he was just trying to fix a bug by inserting some statements and made moer bugs doing that. – Paul Ogilvie Dec 19 '15 at 17:47
  • I see your intention...OK. – Paul Ogilvie Dec 19 '15 at 17:48
  • Yeap, agree. He d better use a debugger. Still you have not convinced me why a) my answer is wrong, and b) why I should not event mention how to change a pointer in a function – sestus Dec 19 '15 at 17:48
1

The condition if (l == NULL) is not true in addToFront because l is NOT NULL here. You have called l = malloc(sizeof(list)); at the beginning of the main which made l not NULL. There is no need to malloc and initialize l in this way. I suppose that by l you meant the head pointer to the list. This should be NULL at beginning (i.e. do not call malloc at main and assign the returned address to l) and your memory for node should be allocated in addToFront like this:

// need to pass **l to update it
void addToFront(int num, list **l) {
    // allocate memory for new node
    list *tmp = (list *) malloc(sizeof(list));

    tmp->value = num;

    // new node should point to whatever head is currently pointing
    // even if head is NULL at beginning
    tmp->next = *l;

    // finally l needs to point to new node
    // thus new node becomes the first node
    *l = tmp;
}

Remove malloc from main.

int main() {
    list *l;
    addToFront(5, &l);  // pass address of l
    printList(l);

    // other stuffs
}

And printing will be like this:

void printList(list * l) {
    list *current = l;
    while (current != NULL) {
        printf("%d ", current->value);
        current = current->next;
    }
}

Finally it is not enough to free only l. You need to traverse whole list and free every node in it.

void freeList(list *l) {
    list *current = l;
    while (current != NULL) {
        list *tmp = current;
        current = current->next;
        free(tmp);
    }
}
taskinoor
  • 45,586
  • 12
  • 116
  • 142
  • This certainly removes the infinite loop, but seems to still print only zero, not 5. Also using list *l throws another exception in visual studio, whereas, using list l works (with zero as the resulting output). At this point It feels like the issue lies with Visual Studio, and not the program itself. – TTEd Dec 19 '15 at 17:54
  • @TTEd, did you fixed print function too? Please go through my code and try to understand the comments. There are a number of places in your code that need to be fixed. – taskinoor Dec 19 '15 at 17:56
  • Yep I fixed it, exactly as you've shown, except **list * l** throws an exception, so I had to use **list l** – TTEd Dec 19 '15 at 17:57
  • I edited my `addToFront`. `tmp->next = l;` should be `tmp->next = *l;`. That * before `l` was missing. – taskinoor Dec 19 '15 at 18:00
  • Okay, this now prints **0 5**. Which half works, since it should only print 5, it being the only and first element? The fact that I'm having use pointers to pointers seems absurd for such a simple linked list, no idea what's going on. – TTEd Dec 19 '15 at 18:04
  • Not sure from where the zero is coming from. Please edit the question and append your current code. Yes, pointer use can be tricky and really need to understand it well to do something like this. – taskinoor Dec 19 '15 at 18:07
  • @TTEd I suspect you still has `initialize(l)` in your `main` which is not required. Probably that's where zero is coming from. – taskinoor Dec 19 '15 at 18:11
  • Without an Initialize function, I get another exception thrown, I've updated the code. – TTEd Dec 19 '15 at 18:14
  • @TTEd with your current code I see three problems: 1. `l` needs to be a pointer `list *l`. 2. `initialize(&l)` is not needed. 3. `printList(l)` `&` is not required since `l` is pointer now. I think you are really missing the concept of pointers. Sometimes a paper and pen is more helpful when dealing with pointer. – taskinoor Dec 19 '15 at 18:17
  • 1.) Using **list * l** and removing initialize causes a **read access violation 0xCCCCCCCC**, which i previously stated in my previous comments. – TTEd Dec 19 '15 at 18:21
  • Okay I've now got it working: I'm now using **list * l**, as you said, BUT I need the initialize function, which I'm calling as such: **initialize(&l);**. And calling the print function as such: **printList(l);** You were right about using a proper pointer, but I without calling the Initialize function, I get thrown several exceptions. – TTEd Dec 19 '15 at 18:27
  • Also any idea why I can't do something like : l->next->value? Where I get the error, "pointer to incomplete class type". – TTEd Dec 19 '15 at 18:33
  • @TTEd Great that you fixed it. Anyway, I was checking your code and figured out why it was crashing without `initialize`. The reason is we need to assign NULL to `l` while defining it `list *l = NULL;`. I missed that part while writing the answer. – taskinoor Dec 19 '15 at 18:34
0

This is because of your printing function. your are only printing the values that have a next node after them. so having only 1 value will not print anything. instead you should have:

void printList(list * l) {

    while (l !=NULL)
    {
    printf("%d ", l->value);
    l=l->next;
    }

}

also in your addToFront function, you have a logical error, you are only setting the data and size IF the list passed in is in fact NULL, which is not the case.

ForeverStudent
  • 2,487
  • 1
  • 14
  • 33
  • This seems to throw a strange exception, " 0xCDCDCDCD.". Also I'm only currently adding one element to the list, (only calling addToFront once), which should mean the list is Null in this case (since I'm only calling it once). – TTEd Dec 19 '15 at 17:35
  • the list will only be NULL if you did not allocate memory for it, which you did in your main function – ForeverStudent Dec 19 '15 at 17:43
  • ***0xCDCDCDCD*** means uninitialized heap memory in Visual Studio: http://stackoverflow.com/a/127404/487892 – drescherjm Dec 19 '15 at 17:48
0

Use:

void addToFront(int num, list **l) {
    list *tmp= malloc(sizeof(list));
    tmp->value = num;
    tmp->next = *l;
    *l= tmp;
}

Note: initialize is not needed. Just pass an empty or non-empty list so main can just do:

int main()
{
    list * l= NULL;
    addToFront(5, &l);
...

See ForeverStudent his solution to fix the print bug.

Paul Ogilvie
  • 25,048
  • 4
  • 23
  • 41