adding nodes to a singly linked list

Question

I am writing a disassembler and am using a linked list to save the data from a symbol file(sym_file). I have been looking at all the other posts but still cannot get it to work! (keep getting segmentation fault) I am trying to append nodes at the the end of the list so I can keep track of the head_node.

void read_symbol_file(char* file_name)
{
    struct node* head_node = new struct node;
    struct node* node = new struct node;
    struct node* temp;
    ifstream sym_file(file_name, ios::out);
    char label[16];
    char the_type[6];
    char address[6];

    if(sym_file.is_open())
    {
        while (!sym_file.eof())
        {
            sym_file >> label;
            sym_file >> the_type;
            sym_file >> address;

            if(strcmp(the_type, "line") == 0)
            {
                node->type = line;
                node->label = label;
                node->address = atoi(address);
            }
            else if(strcmp(the_type, "block") == 0)
            {
                node->type = block;
                node->label = label;
                node->address = atoi(address);
            }
            else if(strcmp(the_type, "ascii") == 0)
            {
                node->type = ascii;
                node->label = label;
                node->address = atoi(address);
            }
            else if(strcmp(the_type, "word") == 0)
            {
                node->type = word;
                node->label = label;
                node->address = atoi(address);
            }
            else
            {
                cout << "invalid label" << endl;
                exit(0);
            }

            if(head_node == NULL)
            { 
                head_node = node;
                head_node->next = node;
            }
            else
            {
                temp = head;
                while(temp->next != NULL)
                    temp = temp->next;
                temp->next = NULL;
            }
        }
        sym_file.close();
    }

    else
    {
        cout << "File does not exist or could not be found." << endl;
        exit(0);
    }
    cout << head_node->label << endl;

}

I wish I had $1 for every time I saw [`while (!sym_file.eof())` incorrectly used on this site](http://stackoverflow.com/q/5605125/78845)! — Johnsyweb, Mar 03 '13 at 11:24
"keep getting segmentation fault". This should help *you* track down the error. Run this code through your debugger to see where you are accessing memory that does not belong to you. Also reduce the size of your functions by doing just one thing (appending to a linked list is a completely different operation from reading a file). If you're still stuck; please post a [Short, Self Contained, Correct (Compilable), Example](http://sscce.org/) and we'll try to assist. — Johnsyweb, Mar 03 '13 at 20:47

Mark Taylor · Answer 1 · 2013-03-03T04:52:32.223

1

This line should be a compiler error:

          temp = head;

because there is no declaration of a variable 'head'

Second problem is that the allocation of 'new node' is outside the loop, so you only have one 'node' that keeps getting overwritten. This line should be moved inside the 'while' loop:

   struct node* node = new struct node;

Third, the else block never assigns a pointer to 'node' to a next variable. Should be

  temp->next = node;

There are several other problems in this code related to head_node, which will not be == NULL once it is allocated. There appears to be some confusion about the difference between assigning the address of a structure to a pointer, and copying the contents of one structure to another structure as in *head_node = *node;

In this case, there is no need to use 'new' to allocate a head_node. That's just a pointer, which can be initialized to NULL.

edited Mar 03 '13 at 04:52

answered Mar 03 '13 at 04:40

Mark Taylor

1,793
13
17

Thanks Mark. For the first revision, I meant to put temp = head_node; I moved the declaration into the while loop and added the temp->next = node into the else statement. I did some more testing and for some reason, temp->next is always NULL and because of that, "cout << head_node->label << endl;" is not printing anything. – nLee Mar 03 '13 at 04:51
sorry didnt see your edit before i reposted. – nLee Mar 03 '13 at 05:00
So I made some changes and my last if-else statement follows: – nLee Mar 03 '13 at 05:18
if(head_node == NULL) { head_node = node; } else { temp = head_node; while(temp->next != NULL) { temp = temp->next; temp->next = node; } } – nLee Mar 03 '13 at 05:20
1

That's still not it. The temp->next = node; needs to be outside the closing } as in {temp = temp->next; } temp->next = node;} Otherwise, as you walk the list you are trashing all the next pointers along the way. I suggest working this problem out at a higher level of abstraction, with drawings of the data structures until you fully understand the algorithm. Treating it just as typos to be corrected doesn't seem to be working. – Mark Taylor Mar 03 '13 at 05:22
sorry I cannot figure out how to format the comments in code form..I also change the inialization of head_node to struct node* head_node = NULL; I got rid of the segmentation fault but I am now back to where I was 4 hours ago. My head_node pointer is nothing more than a tail_pointer. I know this should be an easy fix but Ive been at it since 3(eastern time) and its not seeming that way. – nLee Mar 03 '13 at 05:23
I had previously had it outside the while loop but it was giving me the same result. – nLee Mar 03 '13 at 05:23
I appreciate the help though! – nLee Mar 03 '13 at 05:27
Hey Mark, I have one last quick question if you don't mind: I feel like I have a good grasp on linked lists, but for some reason I cannot figure out how to get the head_node reference to stay pointing to the head_node. It's value shouldn't be changed during execution since I only assign node to head_node once. At the end of the function I print out each nodes value and they all hold the value of the tail node's value. Is there something in my code that is reassigning the head_node's value? – nLee Mar 03 '13 at 05:53
1

head_node->next = node; is not correct. The next pointer should remain NULL to mark the end until a second node is added, in the else clause. We can now look back and see that this probably shouldn't have been a stackoverflow question, since it has turned in to a chat session in the comments. SO questions should be limited to one clearly stated question about a concept that can be answered in a way that will be useful to future readers. This turned into a code review that is not likely help anyone else. – Mark Taylor Mar 03 '13 at 14:19

adding nodes to a singly linked list

1 Answers1