linked list in c (what am I doing wrong here?)

Question

I have this piece of code, and I can't figure out why this isn't working? The inputData() function seems to work, but the print method only prints the first line I send to inputDate() for as many lines that I have inputed.

I'm reading from a file, one line at a time, and putting in to the linked list, that's where the issue is. If I pass values in the code, then there is no problem?

//LINKED LIST
void inputData(char *l)
{
    struct lines *pNewStruct = (struct lines *) malloc(sizeof(struct lines));
    pNewStruct->line = l;

    //inserts if list empty, next set to null
    if(pFirstNode == NULL){
        pNewStruct->next = NULL;
        pFirstNode = pLastNode = pNewStruct;

    } else {

        //inserts if list contains one element
        //this is done to differentiate between first and last node
        if(pFirstNode == pLastNode) {
            pFirstNode->next = pNewStruct;
            pLastNode = pNewStruct;
            pNewStruct->next = NULL;

        //inserts elements when elements in list > 2
        } else {
            pLastNode->next = pNewStruct;
            pNewStruct->next = NULL;
            pLastNode = pNewStruct;

        }
    }
}

void printData()
{
    struct lines *temp = pFirstNode;

    while(temp != NULL)
    {
        printf("%s", temp->line);
        temp = temp->next;
    }
}

You shouldn't cast the return value of malloc in C specifically; it can lead to all sorts of obscure errors. — Aristides, Sep 22 '13 at 16:02
Are you intentionally storing only the pointer to the received string instead of copying the string itself? This is a very easy way to lose your data — Leeor, Sep 22 '13 at 16:04
Yes, the pFirstNode and pLastNode are declared globally. Sorry about that. — Arash Saidi, Sep 22 '13 at 16:06

score 3 · Accepted Answer · answered Sep 22 '13 at 16:07

3

For each line you should dynamically allocate new memory and copy the contents of each line into the newly allocated string. Otherwise, if no one keeps track of the memory allocated for these strings, or the strings are on the stack, you run the risk of losing them.

answered Sep 22 '13 at 16:07

Sebastian-Laurenţiu Plesciuc

2,413
1
24
41

If `inputData("one")` is called, then would the memory for "one" be stack allocated? http://stackoverflow.com/questions/2589949/c-string-literals-where-do-they-go – Rafi Kamal Sep 22 '13 at 16:14
This seems to be the problem, but how do I do this? – Arash Saidi Sep 22 '13 at 16:21
1

@HatoriSanso A combination of `malloc` and `strcpy`. – Dennis Meng Sep 22 '13 at 16:21
1

Ok, the list and everything was in order, the problem was that I was using a string buffer and that messed things up. So I copied the buffer to a char (didn't use malloc, just set the size of the char to be BUFSIZ, since the buffer never exceeds the that, and used strcpy). Sorry that this question was so badly formed, I couldn't figure out what the hell was happening in the first place, so you guys helped a lot! – Arash Saidi Sep 22 '13 at 16:33

score -1 · Answer 2 · answered Sep 22 '13 at 16:22

pNewStruct->line = l;

This won't do. And we never see what pFirstNode really is.

So how about using strcpy for copying the char.

Then the two branches in the else. Why do you think you need them. You initiate the pointers while adding the first element. So this pFirstNode->next = pNewStruct;

Is the same in the first case as pLastNode->next = pNewStruct;

So there is not need for another if an else branch in the outer else.

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

struct lines {
  char line [120];
  struct lines *next;
};

struct lines *pFirstNode = NULL;
struct lines *pLastNode = NULL;


//LINKED LIST
void inputData(char *l)
{
    struct lines *pNewStruct = (struct lines *) malloc(sizeof(struct lines));
    strncpy(pNewStruct->line,l,119);


    //inserts if list empty, next set to null
    if(pFirstNode == NULL){
        pNewStruct->next = NULL;
        pFirstNode = pLastNode = pNewStruct;

    } else {
      pLastNode->next = pNewStruct;
      pNewStruct->next = NULL;
      pLastNode = pNewStruct;
    }      
}

void printData()
{
    struct lines *temp = pFirstNode;

    while(temp != NULL)
    {
        printf("%s", temp->line);
        temp = temp->next;
    }
}


int main(void) {
  inputData("one");
  inputData("two");
  inputData("three");
  printData();
  return 0;
}

`pNewStruct->line=l;` is OK for string literals. That part of your answer is worng I think — CS Pei, Sep 22 '13 at 16:24
I think you are wrong. I'm still puzzling what's the problem with this code should be, maybe someone will enligthen me. — Friedrich, Sep 23 '13 at 05:36

linked list in c (what am I doing wrong here?)

2 Answers2