views:

230

answers:

6

I have a simple assignment that the professor wants us to do. Basically to pull in some numbers from a text file and load into a linked list. I don't want to get to much into the details but I have a basic question.

He provided us with a function like so:

INTLIST* init_intlist( int n ) 
{
INTLIST *lst;
lst = (INTLIST *)malloc(sizeof(INTLIST));
lst->datum = n;
lst->next = NULL;
return lst;
}

This function is used to initialize the linked list with the first element. Then he asked us to define a function with this signature:

int insert_intlist( INTLIST *lst, int n )

So I assume he just wants us to add to the linked list so I tried this:

int insert_intlist( INTLIST *lst, int n )
 {
 INTLIST* lstTemp;
 lstTemp = (INTLIST *)malloc(sizeof(INTLIST));
 lstTemp->datum = n;
 lstTemp->next = lst;
 lst = lstTemp;       
 free(lstTemp);          
 }

So what my thought process was is that it creates a temporary node, assigns the data value (Datum) and assigns the next pointer to point to where the current pointer is pointing at. Then I reassign the main pointer to this newly created temp node.

That way we have for instance 2 nodes:

[New Temp Node] -> [Prev Initialized Node]

When I step through the code it looks great...

Then back in main I have just a function to print the list:

                   while (lst!=NULL)
                      {
                       printf("The value is:%d", lst->datum);
                       lst=lst->next;
                      }

The problem is this only seems to print one digit (namely the first digit that I am reading in from the file, which I think is the last one in the list or at least I thought it was the last one in the list).

But it should keep going through as I have 10 digits in the file. I know the code is very dirty and I will clean it up...here is my entire main function if anyone needs more info:

#include <stdio.h>
#include <stdlib.h>
#include "intlist.h"

int main(int argc, char *argv[])
{
  char c;    /* Character read from the file. */
  FILE* ptr;   /* Pointer to the file. FILE is a
       structure  defined in <stdio.h> */
  int index=0;
  //INTLIST* aList[10]; //will use later

    /* Open the file - no error checking done */
  ptr = fopen("1.txt","r");
    /* Read one character at a time, checking 
       for the End of File. EOF is defined 
      in <stdio.h>  as -1    */

  if(ptr==NULL) {
    printf("Error: can't open file.\n");
    /* fclose(file); DON'T PASS A NULL POINTER TO fclose !! */
    return 1;
  }

  //aList[index] = malloc(sizeof(INTLIST)); WE NEED THIS LATER ON....
  INTLIST *lst=NULL;

  while ((c = fgetc(ptr)) != EOF)
  {
        if (c != ' ') 
        {
         //make sure it isnt a space
         int i = c - '0'; //get the value from the text file
             if(c=='\n') 
                 {
                      // aList[index]=lst;
                      // index++;
                      // aList[index] = malloc(sizeof(INTLIST));

                           while (lst!=NULL)
                              {
                               printf("The value is:%d", lst->datum);
                               lst=lst->next;
                              }

                           free(lst);
                           free(aList[index]);
                           return 0;
                          //new line in the file 
                         //create another linked list
                 }

            if (lst==NULL)
             lst = init_intlist(i);
            else
             insert_intlist( lst, i); 
        }
  }

  fclose(ptr);
  system("PAUSE"); 
  return 0;
}

Here is intlist.h for anyone who may need it:

#ifndef __intlist_h__
#define __intlist_h__
/* each entry in the list contains an int */
typedef struct intlist {
int datum;
struct intlist *next;
} INTLIST;
INTLIST *init_intlist( int n ); /* initializes the intlist with initial datum n */
int insert_intlist( INTLIST *lst, int n ); /* Inserts an int (n) into an intlist from the beginning*/
void list_append(INTLIST *list, void *datum); /* Inserts entry to the end of the list */
INTLIST* list_front(INTLIST *list); /*return the element at the front of the list, and remove it 
from the list*/
void list_map( INTLIST *list, void (*f)(void *) ); /*Applies a function to each element of the list */
void list_delete( INTLIST *list ); /* Deletes (and frees) all entries in the list */
#endif
+3  A: 

This line:

lst = lstTemp;  

Only changes the value of lst inside the function. It won't propagate back to the copy of the pointer that the caller has.

You can either use a pointer-to-a-pointer, or if you can't change the function signature, insert somewhere other than the head of the list.

Though the typical way of handling this is to not point to the first element in the list - rather, you have some sort of list structure that holds a pointer to the first element, and some other information about the list (say, how many elements it has). Then you pass a pointer to that structure around.

Anon.
Ok I understand that it dies after the function so I cannot change the function signature, can you give me more info about insert somewhere other then the head of the list?
You could walk through the list to the end (`while(listNode->next) listNode = listNode->next;`), and then stick it on there (`listNode->next = newNode;`)
Anon.
+4  A: 

A couple of issues here.

I'll start with a BAD bug:

int insert_intlist( INTLIST *lst, int n )
 {
 INTLIST* lstTemp;
 lstTemp = (INTLIST *)malloc(sizeof(INTLIST));
 lstTemp->datum = n;
 lstTemp->next = lst;
 lst = lstTemp;       
 free(lstTemp);             //   <<<<<  NO!
 }

You are still using that memory, so you can't free it.


Secondly, the proto-type supplied to you for insertion has no way to return a new front of the list, so you can not change the front of the list. This implies that you must add new nodes to the back, rather than to the front as you have done.

Also, the supplied return type of int probably means that he expects out the number of nodes in the list, which is no problem as you're going to have to walk the list to find the back anyway.

Have another go at it, you're not doing badly at all.

dmckee
I tried commenting that part and its still the same issue.
ok thank you for the kind words, let me see if i can add after the head of the list?
ok dmckee I am going to try this: int insert_intlist( INTLIST *lst, int n ) { INTLIST* lstTemp; lstTemp = (INTLIST *)malloc(sizeof(INTLIST)); lstTemp->datum = n; INTLIST* pTemp; pTemp=lst; while (pTemp->next != null) { pTemp = pTemp->next; } pTemp->next=lstTemp; lstTemp->next=NULL; }
Wow thanks for the hint that part worked...I will accept as I read through the answers but +1 for giving me a hint and not the answer!
@jmh86: I actually think that Jerry was closer to the mark than I was, alas. I didn't read the comments in the header, but rather went with my intuition.
dmckee
Sorry I was just reading this and...Just one question about this if the text file contains 1 9 7 2 and I he adds the 1 to the linked list and then he adds 9..the issue I see with adding it after the head and swapping values is eventually his linked list is backwards it then becomes 2 7 9 1. Because you are saying add it next to the head and swap data values...so is this really what he should do? It seems like adding it to the end is the way to go? I am only asking because I'm not a C developer but that is how I see it ?
JonH
@JonH: Yes, that function will stack the inputs so that the last input is at the front of the list. If you read the comments in the `intlist.h` file that was (I guess) supplied to the poster, you'll see that this is the semantics expected of that function, and that there is another function that is expected to build the list "in order" by appending at the back.
dmckee
@dmckee awesome thank you for the info. You did mention that there is another function that is expected to build the list in order. But I noticed there is an append function: void list_append(INTLIST *list, void *datum);. I am a C# developer trying to learn C. Could you help me understand why he is passing void* datum? I can see that the function returns void meaning he doesn't need to return anything but how would one call this function, in addition, what is the void* datum supposed to be ? Thanks again awesome info on this thread!
JonH
`void *` is the generic pointer type in C. You can convert any object pointer to `void *` and back without loss of information. In this case, the rest of the code deals with `INTLIST` and not a generic list, so `void *datum` could have been written as `INTLIST *datum`. But `void *` in C is used to mean "a generic pointer type". For example, `malloc()` returns a `void *` because it doesn't know what type of object it is allocating memory for.
Alok
@JonH: I agree with Alok, but would like to note that this is a *very* odd place to see that kind of chicanery. I can't image *why* that particular signature would be specified unless 1) they are going to generalize later, and this represents a copy-n-paste error; 2) the professor plans to trick the students into creating a horrible bug as an object lesson; or 3) just to show that it can be done. Strange, strange, strange.
dmckee
@Alok - ok but I guess why did he choose to do that. How would one call that function being that you have to insert a node to the list it looks like?
JonH
@dmckee see my question to @alok...I guess I agree with you about very odd..it looks like a really bad prof?
JonH
Let's say I have a type `T`. Then, `T data; T *pdata = void *vp = pdata;` gives me a pointer to `void` called `vp`. Now, I can convert this pointer in a function back to `T *`. So, given the above declarations and `void f(void *p) { T *tp = p; }`, I can call `f(vp);`. As dmckee said, the prototype using `void *` is not very useful above, since the rest of the code is not type-agnostic.
Alok
@Alok I understand it its sort of like the C# object *object*.I can see how you are "casting" it or converting it. But my question is the function definition was void list_append(INTLIST* list, void *datum). And from what you said void* datum is really INTLIST* datum. My question is if the prof wants to it looks like insert a node to the list how would one call this function, would a node have to be created in the main function and passed to this append function? It just seems strange the way the function prototype is written. How else could one call this function?
JonH
@JonH: The signature there is `void list_append(INTLIST* list, void* datum)`. Two pointers are passed in. One is the head of the list, the other is a pointer to a location holding unspecified data (that is what `void*` means). The function will interpret that unspecified data somehow of another. In this case it will almost certainly treat it as a `int`. You could call it like this: `INTLIST*l=init_intlist(3); int foo = 5; list_append(l,` resulting in a list holding 3 then 5.
dmckee
@JonH: I don't think that the comments are a good place to hold this kind of discussion. There are a great many questions on c pointer semantics all over SO. Read a few, and if you are still confused, post another question. Lots of people are available to help.
dmckee
@dmckee that now makes perfect sense..wish I could upvote you some more. I do agree that I should stay on topic and ask in SO. So I want to personally thank you for your patience. Alok also thank you to you too! +++++1 to both of you !
JonH
@JonH: Glad to be of help. As dmckee said, there is a lot of good information here on SO if you need more.
Alok
+2  A: 

In C, parameters are passed to functions "by value", meaning they are copied when you enter the function and any changes you make to them are not reflected back to the caller. That means that when you modify lst to point to your newly allocated memory it doesn't actually modify the caller's pointer to the list.

EDIT: As dmckee pointed out, you shouldn't free the memory in your insert function as you are still using it. That's definitely a bug, but it's not the one causing your problem.

Tal Pressman
I guess my question is since I cannot change the function signature..how to handle something like this.
Why not point out the issue in the code to your professor?
AJ
As others have pointed out, you're going to have to insert the new node somewhere else in the list. It can either be at the end (thus keeping the order of the numbers), or you can just add it as the second node.If you want to add it at the end, you're going to have to get to the end of the list, and then make the last node point to the newly allocated one.If you add it second, you have to make sure the new node points to the node that was previously the second node, and then make the first node point to the new one.
Tal Pressman
+4  A: 

Working with code like:

int insert_intlist( INTLIST *lst, int n )
 {
 INTLIST* lstTemp;
 lstTemp = (INTLIST *)malloc(sizeof(INTLIST));
 lstTemp->datum = n;
 lstTemp->next = lst;
 lst = lstTemp;       
 free(lstTemp);          
 }

This has a couple of problems. First of all, the free(lstTemp) seems to be freeing the node that you just inserted in the list, which you probably don't want to do.

Second, you're passing the pointer to the list into the function -- which means the function can't modify that pointer, so when you assign to the pointer, you're only changing your local copy of it.

You have two choices: you can either pass in a pointer to that pointer (so you can modify the original pointer) or you can get clever and figure out a way to avoid needing to (but I won't give away the secret right away...)

Jerry Coffin
I cannot modify the function signature as that is what the professor said we cannot do. I also commented the free(lstTemp) and I still had the same issue.
Yes -- while that's *a* bug, it's not the bug you noticed yet. If you have to use the same function signature, there's still a way, but it's a tad more tricky. The basic idea is that you add a new node *after* the current head of the list, but then swap the values between the nodes to get them back in order.
Jerry Coffin
@Jerry: since the return type of the function is `int`, maybe the instructor wants the function to return the old head's payload, and replace it with the new value, thus making sense of the return type in the prototype, and simplifying the function considerably! :-)
Alok
Just one question about this if the text file contains 1 9 7 2 and I he adds the 1 to the linked list and then he adds 9..the issue I see with adding it after the head and swapping values is eventually his linked list is backwards it then becomes 2 7 9 1. Because you are saying add it next to the head and swap data values...so is this really what he should do? It seems like adding it to the end is the way to go? I am only asking because I'm not a C developer but that is how I see it ?
JonH
@JonH:It depends on what order he wants. He was originally trying to add items as the new head item, so I was telling him a way he could maintain the order that would produce. It's also possible to add items to the end, but it gives the opposite order, and takes longer. If you really don't care about the order, you can add each item immediately after the head of the list, and don't bother swapping. This gives a rather strange order: the first item first, and the rest in reverse order. Then again, if you don't care about order, you shouldn't use a linked list to start with.
Jerry Coffin
@Jerry thank you for the clarification I was just asking to become more familiar with C.I am a C# developer and ran across this post.I assumed when you meant swapping values that the order would make it the same and it really doesn't. But that is because I did not understand it so no issue with your explanation. One thing I did notice is alok mentioned in his thread this: "In C, everything is passed by value. If you want function to change something you need to pass its address to the function".My question is the parameter is *lst isn't that a pointer?I could not understand why **lst is ned?
JonH
@JonH:yes, it is a pointer. That lets you change what the pointer points at -- but in this case, what you need to change is the pointer itself, and to do that, you'd need a pointer To that pointer.
Jerry Coffin
@Jerry - +1 you are the man, if only I had a C mentor @ work :)
JonH
+2  A: 
Alok
Nice and very well put. Will help anyone understand C :).
JonH
A: 

I Agree with Alok. I Have the same problem/ Professor. I am new to C programming and I have been looking all over the web for forms and C webpages for help. I have came across a source that supports Alok.

I used

INTLIST *list_add(INTLIST **p, int i){

INTLIST *n;
    n = (INTLIST *) malloc(sizeof(INTLIST)); 
        if (n == NULL) 
    return NULL;   
    n->next = *p; /* the previous element (*p) now becomes the "next" element */

     *p = n;       /* add new empty element to the front (head) of the list */

      n->datum = i;
    return p; }

From my main I can pass in

INTLIST *list

list_add(&list, 1); list_add(&list, 2);

so when i print the list it prints 2 1

The professor suggested this:

INTLIST *mylist[N];

Where N is the number of rows of your input file. Then mylist[i] is a pointer to the ith linked list.

Okay Fine: create for testing purposes INTLIST *mylist[2];

I call the same functions:

list_add(&list[0], 1); list_add(&list[0], 2);

This prints out 2 1 ... Great,

But when I do this:

list_add(&list[1], 3); list_add(&list[1], 4);

I get a Segmentation fault..

MRP
The Segmentation fault comes when I try to print the list not during the list_add();
MRP