problem with feof ?

Discussion in 'C Programming' started by mark.babli@gmail.com, Jan 28, 2013.

  1. Guest

    hey all,

    did c a while back, trying to get back into it. the following stub of code fails on the while(!feof(fp)) for some reason. could someone help? Thanks
    Please note that all variables here are indeed declared:

    do
    {
    fgets(line,80,fp);
    tmpNode = (struct dlnode*)malloc(sizeof(struct dlnode));
    if(tmpNode == NULL)
    {
    perror("unable to allocate memory");
    intResult= -2;
    }
    sscanf(line,"%d:%s",&intVal,strVal,100);
    strcpy(tmpNode->chrName,strVal);
    tmpNode->intID = intVal;
    if(addDoubleNode(tmpNode)<0)
    {
    perror("unable to create new node...error code (-3)");
    exit(-3);
    }
    }while(!feof(fp)); //On this line the code fails.

    Thanks again
     
    , Jan 28, 2013
    #1
    1. Advertising

  2. Joe Pfeiffer Guest

    writes:

    > hey all,
    >
    > did c a while back, trying to get back into it. the following stub of code fails on the while(!feof(fp)) for some reason. could someone help? Thanks
    > Please note that all variables here are indeed declared:
    >
    > do
    > {
    > fgets(line,80,fp);
    > tmpNode = (struct dlnode*)malloc(sizeof(struct dlnode));
    > if(tmpNode == NULL)
    > {
    > perror("unable to allocate memory");
    > intResult= -2;
    > }
    > sscanf(line,"%d:%s",&intVal,strVal,100);
    > strcpy(tmpNode->chrName,strVal);
    > tmpNode->intID = intVal;
    > if(addDoubleNode(tmpNode)<0)
    > {
    > perror("unable to create new node...error code (-3)");
    > exit(-3);
    > }
    > }while(!feof(fp)); //On this line the code fails.
    >
    > Thanks again


    What's the symptom? In what sense do you mean "fails"? segfault? some
    error that turns up in the debugger? laser blasts from Martians?
     
    Joe Pfeiffer, Jan 28, 2013
    #2
    1. Advertising

  3. On 2013-01-28, <> wrote:
    >
    > do
    > {
    > fgets(line,80,fp);
    > tmpNode = (struct dlnode*)malloc(sizeof(struct dlnode));
    > if(tmpNode == NULL)
    > {
    > perror("unable to allocate memory");
    > intResult= -2;
    > }
    > sscanf(line,"%d:%s",&intVal,strVal,100);
    > strcpy(tmpNode->chrName,strVal);
    > tmpNode->intID = intVal;
    > if(addDoubleNode(tmpNode)<0)
    > {
    > perror("unable to create new node...error code (-3)");
    > exit(-3);
    > }
    > }while(!feof(fp)); //On this line the code fails.


    It looks OK as far as I can see subject to the provisos:

    a) We can't see the entire program, nor even the definition of
    relevant types and functions. Therefore we must simply take those
    on trust.

    b) We have no idea what you expect to happen.

    c) We have no idea what really is happening. The conditional
    controlling a loop essentially has two possible outcomes barring
    side effects: the loop is either continued or it isn't. Neither
    outcome is a "failure".

    What is really happening and how does this compare to what you
    expect to happen. You answer should include the exact input the
    code is considering.

    --
    Andrew Smallshaw
     
    Andrew Smallshaw, Jan 28, 2013
    #3
  4. writes:

    > hey all,
    >
    > did c a while back, trying to get back into it. the following stub
    > of code fails on the while(!feof(fp)) for some reason.


    "Fails" is a very general description. What happens?

    > could someone
    > help? Thanks Please note that all variables here are indeed declared:


    That they are declared is not enough! See my comments below.

    > do
    > {
    > fgets(line,80,fp);


    How is line declared? There are declarations that will make this line
    go horribly wrong! Also, if the call fails you carry on regardless with
    indeterminate data in the buffer.

    > tmpNode = (struct dlnode*)malloc(sizeof(struct dlnode));


    tmpNode = malloc(sizeof *tmpNode);

    is neater and avoids some of the repetition.

    > if(tmpNode == NULL)
    > {
    > perror("unable to allocate memory");
    > intResult= -2;
    > }


    You carry on even when tmpNode == NULL. If that does ever happen, what
    follows will wreak havoc.

    > sscanf(line,"%d:%s",&intVal,strVal,100);


    The 100 does nothing. Is it supposed to be a buffer size? If so, you
    can put the size in the format string and without such a size limit the
    sscanf call can overflow the buffer. Also, you don't test to see if
    this scan call succeeded. If it fails, the code below could do much
    damage.

    > strcpy(tmpNode->chrName,strVal);


    Ideally, you'd post the declaration of the struct. This could be very
    wrong indeed, but I'm guessing that chrName is an array but even then
    you don't do anything to protect again too much data being copied.

    > tmpNode->intID = intVal;
    > if(addDoubleNode(tmpNode)<0)


    This function might be doing something wrong as well. Ideally, post a
    small complete program that demonstrates the problem.

    > {
    > perror("unable to create new node...error code (-3)");
    > exit(-3);
    > }
    > }while(!feof(fp)); //On this line the code fails.


    It's possible that some previous error is causing the problem. The
    general point is that you are not testing for errors (or success where
    that is moire useful) and you are not protecting against buffer
    overflows.

    In general, input loops in C are better written without using feof. The
    traditional style is to loop while the input is successful:

    while (fgets(line, 80, fp) &&
    sscanf(line,"%d:%99s", &intVal, strVal) == 2) {

    /* there was a line (though maybe one that was too long) and it
    had the right sort of data in it so we can safely proceed to
    process it. */
    }

    /* You can certainly use feof here to check that all went well. */

    --
    Ben.
     
    Ben Bacarisse, Jan 28, 2013
    #4
  5. On Sun, 27 Jan 2013 20:08:52 -0800 (PST), wrote:

    >hey all,
    >
    > did c a while back, trying to get back into it. the following stub of code fails on the while(!feof(fp)) for some reason. could someone help? Thanks
    >Please note that all variables here are indeed declared:


    feof() does not tell you that you have read the last character. It
    tells you that you have tried to read past the last character.

    If the last line in the file is terminated with a \n, then feof
    returns 0 and the loop will iterate one more time. In the loop, the
    call to fgets will fail leaving line unchanged. You do not test for
    the failure and process the last line a second time.

    There are other reasons fgets can fail and you don't test for that
    either.

    >
    > do
    > {
    > fgets(line,80,fp);
    > tmpNode = (struct dlnode*)malloc(sizeof(struct dlnode));
    > if(tmpNode == NULL)
    > {
    > perror("unable to allocate memory");
    > intResult= -2;
    > }
    > sscanf(line,"%d:%s",&intVal,strVal,100);
    > strcpy(tmpNode->chrName,strVal);
    > tmpNode->intID = intVal;
    > if(addDoubleNode(tmpNode)<0)
    > {
    > perror("unable to create new node...error code (-3)");
    > exit(-3);
    > }
    > }while(!feof(fp)); //On this line the code fails.


    Yes, the code fails, not the function. You are using it incorrectly.
    You would do better to combine the two calls to fgets and change the
    do-while to
    while (fgets(line, 80, fp) {...}
    and then determine why the loop terminated (end of file being normal
    and anything else being an error).
    >
    >Thanks again


    --
    Remove del for email
     
    Barry Schwarz, Jan 28, 2013
    #5
  6. James Kuyper Guest

    On 01/27/2013 11:08 PM, wrote:
    > hey all,
    >
    > did c a while back, trying to get back into it. the following stub of code fails on the while(!feof(fp)) for some reason. could someone help? Thanks
    > Please note that all variables here are indeed declared:


    Keep in mind that if you do not know why something is failing, you can't
    be sure what's relevant to the fact that it's failing. You should
    provide a complete compilable program demonstrating your problem, not
    just a few lines of code. The code where you actually observed the
    failure is probably far too complicated, so you should simplify the code
    as much as possible before showing it to us, removing any parts that
    leave the problem intact. But after you've simplified it, it should
    still be a complete compilable program - confirm this by compiling it
    before you post it. It must still demonstrate the same problem you're
    asking about - also confirm this before posting your message.

    For a program like the following that does I/O, it's essential that you
    give us an example of inputs that actually trigger the problem.

    > do
    > {
    > fgets(line,80,fp);


    You make no attempt to check whether fgets() succeeded. If it fails due
    to an I/O error, the contents of 'line' are indeterminate. If it reads
    the last line of the file, the end-of-file status will not be set until
    until the next call to fgets(), and that call will leave the contents of
    'line' unchanged, so your program will process the last line of code twice.

    > tmpNode = (struct dlnode*)malloc(sizeof(struct dlnode));
    > if(tmpNode == NULL)
    > {
    > perror("unable to allocate memory");
    > intResult= -2;
    > }


    You check whether tmpNode is null - that's good. You produce an
    appropriate message - that's good. Then you continue processing with the
    null value of tmpNode. That's very bad. Since you didn't see that
    message, this probably isn't the problem your seeing, but it is the
    wrong way to use malloc().

    > sscanf(line,"%d:%s",&intVal,strVal,100);


    You make no attempt to determine whether sscanf() succeeded. It could
    fail if the line is not of the specified form, in which case the
    contents of intVal and strVal will be indeterminate.

    If tmpNode is null, each of next two line, will have undefined behavior.
    Depending upon what addDoubleNode() does with it's argument, the
    behavior of that function is likely to also be undefined.


    > strcpy(tmpNode->chrName,strVal);
    > tmpNode->intID = intVal;
    > if(addDoubleNode(tmpNode)<0)
    > {
    > perror("unable to create new node...error code (-3)");
    > exit(-3);
    > }
    > }while(!feof(fp)); //On this line the code fails.


    In C standard I/O, the end of a file is detected after a failed attempt
    to read it, therefore, it's more appropriate to use an overall structure
    like the following:

    while(fgets(line, 80, fp) == line)
    {
    // Process line.
    }
    if(ferror(fp))
    {
    // handle I/O error
    }
    else
    {
    // End of file reached - if any special.
    // handling is needed, put it here
    }

    While I have found many problems with your program, I can't be sure
    whether any of them are relevant to the failure you observed. You should
    identify what inputs were given to the program, what you expected to
    happen when you gave them, and what actually happened. "the code fails"
    is very nearly useless information, without details about what you mean
    by "fails".
    --
    James Kuyper
     
    James Kuyper, Jan 28, 2013
    #6
  7. wrote:
    > hey all,
    >
    > did c a while back, trying to get back into it.
    > the following stub of code fails on the while(!feof(fp)) for some
    > reason. could someone help? Thanks


    (snip)

    > do
    > {
    > fgets(line,80,fp);


    (snip)
    > }while(!feof(fp)); //On this line the code fails.


    Normally it shouldn't fail, but this all looks very suspicious.

    In Pascal, and maybe some other languages, your are supposed to
    check that you haven't reached EOF before reading.

    In C, the EOF is only reported after a read tried and failed
    (due to reaching EOF).

    -- glen
     
    glen herrmannsfeldt, Jan 28, 2013
    #7
  8. Shao Miller Guest

    On 1/27/2013 23:08, wrote:
    > hey all,
    >
    > did c a while back, trying to get back into it. the following stub of code fails on the while(!feof(fp)) for some reason. could someone help? Thanks
    > Please note that all variables here are indeed declared:
    >


    Share:
    - The structure type definition for 'struct dlnode'.
    - The declaration of 'line'.
    - The declaration of 'strVal'.

    Check the return value of:
    - 'fgets'
    - 'sscanf'

    --
    - Shao Miller
    --
    "Thank you for the kind words; those are the kind of words I like to hear.

    Cheerily," -- Richard Harter
     
    Shao Miller, Jan 28, 2013
    #8
  9. writes:
    > did c a while back, trying to get back into it. the following stub
    > of code fails on the while(!feof(fp)) for some reason. could someone
    > help? Thanks Please note that all variables here are indeed declared:
    >
    > do
    > {
    > fgets(line,80,fp);
    > tmpNode = (struct dlnode*)malloc(sizeof(struct dlnode));
    > if(tmpNode == NULL)
    > {
    > perror("unable to allocate memory");
    > intResult= -2;
    > }
    > sscanf(line,"%d:%s",&intVal,strVal,100);
    > strcpy(tmpNode->chrName,strVal);
    > tmpNode->intID = intVal;
    > if(addDoubleNode(tmpNode)<0)
    > {
    > perror("unable to create new node...error code (-3)");
    > exit(-3);
    > }
    > }while(!feof(fp)); //On this line the code fails.


    As others have pointed out, you're not telling us *how* the
    code fails. There happen to be enough obvious problems with
    the incomplete code you've shown us that we can help you, but in
    general it's important to tell us exactly *how* it fails: what
    input you gave the program, what output it produced, what you
    expected it to produce, and how they differ. http://sscce.org/
    has some good advice.

    All C input functions return a result that tell you whether they
    succeeded or not. You need to use that result, not feof(), to
    decide when to termainte the loop.

    An input function can fail because it encountered an end-of-file
    condition, because of an I/O error, or, in some cases, because of
    incorrectly formatted input (e.g., `scanf("%d", &n)` with input
    "notaninteger"). Different functions indicate this in different
    ways; see the documentation for each function. If you're on a
    Unix-like system, `man fgets` and `man sscanf` should tell you
    everything you need to know.

    The feof() and ferror() functions can be used *after* an input
    function has reported failure to tell you whether it was because
    you reached the end of the file or because of an error. Only one
    will return a true result -- which means that your program will
    loop infinitely if fgets() fails due to an I/O error.

    Section 12 of the excellent comp.lang.c FAQ, <http://www.c-faq.com/>,
    discusses the use of C standard I/O.

    --
    Keith Thompson (The_Other_Keith) <http://www.ghoti.net/~kst>
    Working, but not speaking, for JetHead Development, Inc.
    "We must do something. This is something. Therefore, we must do this."
    -- Antony Jay and Jonathan Lynn, "Yes Minister"
     
    Keith Thompson, Jan 28, 2013
    #9
  10. Guest

    you are right, I am sorry! VS immediately jumps after the first loop to an internal "file-lock" function (the code is very complex, and most of it in assembly). and I get an Error "Read Access Violation "

    On Sunday, January 27, 2013 10:34:19 PM UTC-6, Joe Pfeiffer wrote:
    > writes:
    >
    >
    >
    > > hey all,

    >
    > >

    >
    > > did c a while back, trying to get back into it. the following stub of code fails on the while(!feof(fp)) for some reason. could someone help? Thanks

    >
    > > Please note that all variables here are indeed declared:

    >
    > >

    >
    > > do

    >
    > > {

    >
    > > fgets(line,80,fp);

    >
    > > tmpNode = (struct dlnode*)malloc(sizeof(struct dlnode));

    >
    > > if(tmpNode == NULL)

    >
    > > {

    >
    > > perror("unable to allocate memory");

    >
    > > intResult= -2;

    >
    > > }

    >
    > > sscanf(line,"%d:%s",&intVal,strVal,100);

    >
    > > strcpy(tmpNode->chrName,strVal);

    >
    > > tmpNode->intID = intVal;

    >
    > > if(addDoubleNode(tmpNode)<0)

    >
    > > {

    >
    > > perror("unable to create new node...error code (-3)");

    >
    > > exit(-3);

    >
    > > }

    >
    > > }while(!feof(fp)); //On this line the code fails.

    >
    > >

    >
    > > Thanks again

    >
    >
    >
    > What's the symptom? In what sense do you mean "fails"? segfault? some
    >
    > error that turns up in the debugger? laser blasts from Martians?
     
    , Jan 29, 2013
    #10
  11. Guest

    The program simply reads a file and builds a linked list, each line being anode. the first time the program loops is good, the second time it loops.....I get Access Error violation and I get a very complex code (mostly in assembly) about file_lock function.

    I could post the code - it will be rather long - if I have to. I am trying to re-learn standard C, so, the code isn't that confidential.
     
    , Jan 29, 2013
    #11
  12. Ian Collins Guest

    wrote:
    > The program simply reads a file and builds a linked list, each line
    > being a node. the first time the program loops is good, the second
    > time it loops.....I get Access Error violation and I get a very
    > complex code (mostly in assembly) about file_lock function.
    >
    > I could post the code - it will be rather long - if I have to. I am
    > trying to re-learn standard C, so, the code isn't that confidential.


    Please wrap your lines to 70-80 characters and avoid double spacing your
    quotes.

    Did you address any (preferably all) of the issues in the earlier replies?

    --
    Ian Collins
     
    Ian Collins, Jan 29, 2013
    #12
  13. Guest

    Ben,
    I appreciate the way you explained it. taught me alot. I did indeed findthe problem following your instructions (the problem was indeed somewhere else. The function which builds the list doesn't set the next and prev, so,the root node insertion succeeds, but, anything after failed...

    here is the code after the one line fix (item->next = item->prev = NULL)

    int addDoubleNode(dlnode* item){
    bool isLast = true;
    dlnode* current;
    current = dlRootNode;
    item->next = item->prev = NULL;

    if(current==NULL){
    // handle first node case
    dlRootNode = item;
    current = dlRootNode;
    return(1);
    }else{
    while(current!= NULL){
    // handle middle insertion
    if(strcmp( item->chrName, current->chrName) < 0){
    // we need to insert before (because we are comparing the new item's string first).
    isLast = false;

    if(current->prev != NULL){
    // not the first
    item->prev = current->prev;
    current->prev->next = item;
    item->next = current;
    }else{
    item->next = current;
    current->prev = item;
    }
    if(current->next != NULL){
    current->next->prev = item;
    }
    return(2);
    }
    current = current->next;
    }

    // finally, insert last of the list
    current = dlRootNode;
    while(current->next != NULL){
    current = current->next;
    }
    current->next = item;
    item->prev = current;
    current = item;
    return(3);
    }
    return -1;
    }


    Any recommendations on how to make this code more efficient?

    On Sunday, January 27, 2013 10:57:16 PM UTC-6, Ben Bacarisse wrote:
    > writes:
    >
    >
    >
    > > hey all,

    >
    > >

    >
    > > did c a while back, trying to get back into it. the following stub

    >
    > > of code fails on the while(!feof(fp)) for some reason.

    >
    >
    >
    > "Fails" is a very general description. What happens?
    >
    >
    >
    > > could someone

    >
    > > help? Thanks Please note that all variables here are indeed declared:

    >
    >
    >
    > That they are declared is not enough! See my comments below.
    >
    >
    >
    > > do

    >
    > > {

    >
    > > fgets(line,80,fp);

    >
    >
    >
    > How is line declared? There are declarations that will make this line
    >
    > go horribly wrong! Also, if the call fails you carry on regardless with
    >
    > indeterminate data in the buffer.
    >
    >
    >
    > > tmpNode = (struct dlnode*)malloc(sizeof(struct dlnode));

    >
    >
    >
    > tmpNode = malloc(sizeof *tmpNode);
    >
    >
    >
    > is neater and avoids some of the repetition.
    >
    >
    >
    > > if(tmpNode == NULL)

    >
    > > {

    >
    > > perror("unable to allocate memory");

    >
    > > intResult= -2;

    >
    > > }

    >
    >
    >
    > You carry on even when tmpNode == NULL. If that does ever happen, what
    >
    > follows will wreak havoc.
    >
    >
    >
    > > sscanf(line,"%d:%s",&intVal,strVal,100);

    >
    >
    >
    > The 100 does nothing. Is it supposed to be a buffer size? If so, you
    >
    > can put the size in the format string and without such a size limit the
    >
    > sscanf call can overflow the buffer. Also, you don't test to see if
    >
    > this scan call succeeded. If it fails, the code below could do much
    >
    > damage.
    >
    >
    >
    > > strcpy(tmpNode->chrName,strVal);

    >
    >
    >
    > Ideally, you'd post the declaration of the struct. This could be very
    >
    > wrong indeed, but I'm guessing that chrName is an array but even then
    >
    > you don't do anything to protect again too much data being copied.
    >
    >
    >
    > > tmpNode->intID = intVal;

    >
    > > if(addDoubleNode(tmpNode)<0)

    >
    >
    >
    > This function might be doing something wrong as well. Ideally, post a
    >
    > small complete program that demonstrates the problem.
    >
    >
    >
    > > {

    >
    > > perror("unable to create new node...error code (-3)");

    >
    > > exit(-3);

    >
    > > }

    >
    > > }while(!feof(fp)); //On this line the code fails.

    >
    >
    >
    > It's possible that some previous error is causing the problem. The
    >
    > general point is that you are not testing for errors (or success where
    >
    > that is moire useful) and you are not protecting against buffer
    >
    > overflows.
    >
    >
    >
    > In general, input loops in C are better written without using feof. The
    >
    > traditional style is to loop while the input is successful:
    >
    >
    >
    > while (fgets(line, 80, fp) &&
    >
    > sscanf(line,"%d:%99s", &intVal, strVal) == 2) {
    >
    >
    >
    > /* there was a line (though maybe one that was too long) and it
    >
    > had the right sort of data in it so we can safely proceed to
    >
    > process it. */
    >
    > }
    >
    >
    >
    > /* You can certainly use feof here to check that all went well. */
    >
    >
    >
    > --
    >
    > Ben.
     
    , Jan 29, 2013
    #13
  14. Guest

    Thank you James for you valuable input. While the program was fixed after I set the pointers (next and prev) to null, your advice is very useful and I think you again for it.
     
    , Jan 29, 2013
    #14
  15. Guest

    I did apply all suggestions and will carry them in my code moving forward.
    Thanks
     
    , Jan 29, 2013
    #15
  16. On Mon, 28 Jan 2013 18:34:15 -0800 (PST), wrote:

    >Thank you James for you valuable input. While the program was fixed after I set the pointers (next and prev) to null,
    >your advice is very useful and I think you again for it.


    Since you apparently fixed something in a block of code you have not
    shown us, the errors with this block of code are still there.

    --
    Remove del for email
     
    Barry Schwarz, Jan 29, 2013
    #16
  17. On Mon, 28 Jan 2013 17:48:05 -0800 (PST), wrote:

    >Ben,
    > I appreciate the way you explained it. taught me alot. I did indeed find the problem following your instructions (the problem was indeed somewhere else. The function which builds the list doesn't set the next and prev, so, the root node insertion succeeds, but, anything after failed...
    >
    >here is the code after the one line fix (item->next = item->prev = NULL)
    >
    >int addDoubleNode(dlnode* item){
    > bool isLast = true;
    > dlnode* current;
    > current = dlRootNode;
    > item->next = item->prev = NULL;
    >
    > if(current==NULL){
    > // handle first node case
    > dlRootNode = item;
    > current = dlRootNode;


    current is a local variable in this function. It disappears when the
    function exits. Why set it to a value and then exit?

    > return(1);
    > }else{
    > while(current!= NULL){
    > // handle middle insertion
    > if(strcmp( item->chrName, current->chrName) < 0){
    > // we need to insert before (because we are comparing the new item's string first).
    > isLast = false;


    You never test isLast.

    > if(current->prev != NULL){
    > // not the first
    > item->prev = current->prev;
    > current->prev->next = item;
    > item->next = current;


    Since item->next points to current, shouldn't current->prev point to
    item?

    > }else{
    > item->next = current;
    > current->prev = item;
    > }
    > if(current->next != NULL){
    > current->next->prev = item;


    This looks wrong. item is being inserted in front of current.
    current->prev should point to item. *(current->next) should be
    unchanged.

    > }


    Combining the two previous comments, this entire if statement should
    be replace with
    current->prev = item;

    > return(2);
    > }
    > current = current->next;
    > }
    >
    > // finally, insert last of the list
    > current = dlRootNode;
    > while(current->next != NULL){
    > current = current->next;


    You have already walked down the entire list once to determine that
    item belongs at the end. It seems a duplication of effort to walk
    down it again.

    > }
    > current->next = item;
    > item->prev = current;
    > current = item;


    Again, why set a variable that disappears at the next statement?

    > return(3);
    > }
    > return -1;


    Is there a path through the function that will allow this statement to
    execute?

    >}
    >
    >
    >Any recommendations on how to make this code more efficient?


    --
    Remove del for email
     
    Barry Schwarz, Jan 29, 2013
    #17
  18. Guest

    Hey Barry,

    your comments are true - I've updated my code. However, I want to point outthat your last comment related to traversing the list twice confused me a bit!
    The variable *current will not be set as a result of the loop to the last node, rather, it will point to NULL!
    Simply setting the current->next = item; item->prev = current is not going to work. I believe the while condition should be changed in order to always point to the last item in the list.


    Thank you again for the valuable recommendations.

    On Monday, January 28, 2013 10:23:35 PM UTC-6, Barry Schwarz wrote:
    > On Mon, 28 Jan 2013 17:48:05 -0800 (PST), wrote:
    >
    >
    >
    > >Ben,

    >
    > > I appreciate the way you explained it. taught me alot. I did indeed find the problem following your instructions (the problem was indeed somewhere else. The function which builds the list doesn't set the next and prev, so, the root node insertion succeeds, but, anything after failed...

    >
    > >

    >
    > >here is the code after the one line fix (item->next = item->prev = NULL)

    >
    > >

    >
    > >int addDoubleNode(dlnode* item){

    >
    > > bool isLast = true;

    >
    > > dlnode* current;

    >
    > > current = dlRootNode;

    >
    > > item->next = item->prev = NULL;

    >
    > >

    >
    > > if(current==NULL){

    >
    > > // handle first node case

    >
    > > dlRootNode = item;

    >
    > > current = dlRootNode;

    >
    >
    >
    > current is a local variable in this function. It disappears when the
    >
    > function exits. Why set it to a value and then exit?
    >
    >
    >
    > > return(1);

    >
    > > }else{

    >
    > > while(current!= NULL){

    >
    > > // handle middle insertion

    >
    > > if(strcmp( item->chrName, current->chrName) < 0){

    >
    > > // we need to insert before (because we are comparing the new item'sstring first).

    >
    > > isLast = false;

    >
    >
    >
    > You never test isLast.
    >
    >
    >
    > > if(current->prev != NULL){

    >
    > > // not the first

    >
    > > item->prev = current->prev;

    >
    > > current->prev->next = item;

    >
    > > item->next = current;

    >
    >
    >
    > Since item->next points to current, shouldn't current->prev point to
    >
    > item?
    >
    >
    >
    > > }else{

    >
    > > item->next = current;

    >
    > > current->prev = item;

    >
    > > }

    >
    > > if(current->next != NULL){

    >
    > > current->next->prev = item;

    >
    >
    >
    > This looks wrong. item is being inserted in front of current.
    >
    > current->prev should point to item. *(current->next) should be
    >
    > unchanged.
    >
    >
    >
    > > }

    >
    >
    >
    > Combining the two previous comments, this entire if statement should
    >
    > be replace with
    >
    > current->prev = item;
    >
    >
    >
    > > return(2);

    >
    > > }

    >
    > > current = current->next;

    >
    > > }

    >
    > >

    >
    > > // finally, insert last of the list

    >
    > > current = dlRootNode;

    >
    > > while(current->next != NULL){

    >
    > > current = current->next;

    >
    >
    >
    > You have already walked down the entire list once to determine that
    >
    > item belongs at the end. It seems a duplication of effort to walk
    >
    > down it again.
    >
    >
    >
    > > }

    >
    > > current->next = item;

    >
    > > item->prev = current;

    >
    > > current = item;

    >
    >
    >
    > Again, why set a variable that disappears at the next statement?
    >
    >
    >
    > > return(3);

    >
    > > }

    >
    > > return -1;

    >
    >
    >
    > Is there a path through the function that will allow this statement to
    >
    > execute?
    >
    >
    >
    > >}

    >
    > >

    >
    > >

    >
    > >Any recommendations on how to make this code more efficient?

    >
    >
    >
    > --
    >
    > Remove del for email
     
    , Jan 29, 2013
    #18
  19. Willem Guest

    wrote:
    ) hey all,
    )
    ) did c a while back, trying to get back into it. the following stub of
    ) code fails on the while(!feof(fp)) for some reason. could someone
    ) help? Thanks Please note that all variables here are indeed declared:
    )
    ) do
    ) {
    ) fgets(line,80,fp);
    ) tmpNode = (struct dlnode*)malloc(sizeof(struct dlnode));
    ) if(tmpNode == NULL)
    ) {
    ) perror("unable to allocate memory");
    ) intResult= -2;
    ) }
    ) sscanf(line,"%d:%s",&intVal,strVal,100);
    ) strcpy(tmpNode->chrName,strVal);
    ) tmpNode->intID = intVal;
    ) if(addDoubleNode(tmpNode)<0)
    ) {
    ) perror("unable to create new node...error code (-3)");
    ) exit(-3);
    ) }
    ) }while(!feof(fp)); //On this line the code fails.

    That's not how you use feof.

    You need to test the return code of fgets, and when that indicates an
    error, you can use feof to determine if that error was end-of-file.


    SaSW, Willem
    --
    Disclaimer: I am in no way responsible for any of the statements
    made in the above text. For all I know I might be
    drugged or something..
    No I'm not paranoid. You all think I'm paranoid, don't you !
    #EOT
     
    Willem, Jan 29, 2013
    #19
  20. Tim Rentsch Guest

    writes:

    > [..snip.. here is the revised code]


    Please use spaces rather than tabs in news postings.

    > int addDoubleNode(dlnode* item){
    > bool isLast = true;
    > dlnode* current;
    > current = dlRootNode;
    > item->next = item->prev = NULL;
    >
    > if(current==NULL){
    > // handle first node case
    > dlRootNode = item;
    > current = dlRootNode;
    > return(1);
    > }else{
    > while(current!= NULL){
    > // handle middle insertion
    > if(strcmp( item->chrName, current->chrName) < 0){
    > // we need to insert before (because we are comparing
    > // the new item's string first).
    > isLast = false;
    >
    > if(current->prev != NULL){
    > // not the first
    > item->prev = current->prev;
    > current->prev->next = item;
    > item->next = current;
    > }else{
    > item->next = current;
    > current->prev = item;
    > }
    > if(current->next != NULL){
    > current->next->prev = item;
    > }
    > return(2);
    > }
    > current = current->next;
    > }
    >
    > // finally, insert last of the list
    > current = dlRootNode;
    > while(current->next != NULL){
    > current = current->next;
    > }
    > current->next = item;
    > item->prev = current;
    > current = item;
    > return(3);
    > }
    > return -1;
    > }
    >
    >
    > Any recommendations on how to make this code more efficient?


    Whenever I see a function like this I know there is something
    wrong with it, because of how much code is invested for doing
    such a minor task.

    Conceptually what we want to do is find the appropriate
    insertion point, and insert the new item at that point. To
    make the insertion code simpler, we can keep track of what
    the previous item is (starting off with NULL), and which
    pointer field needs to be updated to point to the new item.
    If we do that we get a much shorter and simpler function
    [disclaimer: not compiled]:

    int
    addDoubleNode( dlnode* item ){
    dlnode *previous = NULL, **link = &dlRootNode;

    while( *link && strcmp( item->chrName, (*link)->chrName ) >= 0 ){
    previous = *link, link = &previous->next;
    }

    item->prev = previous, item->next = *link;
    *link = item;
    if( item->next ) item->next->prev = item;

    return item->next ? 2 : item->prev ? 3 : 1;
    }

    Notice the return expression. If there is an element after item, we
    know we added item before the end of a non-empty list. Otherwise,
    if there is an element before item, then item was added to the end
    of a non-empty list. If neither of the foregoing is true, then item
    was added to an empty list.
     
    Tim Rentsch, Jan 29, 2013
    #20
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Mantorok Redgormor

    feof usage

    Mantorok Redgormor, Sep 20, 2003, in forum: C Programming
    Replies:
    28
    Views:
    1,146
    Dave Thompson
    Sep 29, 2003
  2. Joriveek

    feof

    Joriveek, Sep 13, 2005, in forum: C Programming
    Replies:
    8
    Views:
    709
    Keith Thompson
    Sep 14, 2005
  3. question about gets() and feof().

    , Sep 16, 2005, in forum: C Programming
    Replies:
    3
    Views:
    360
    Simon Biber
    Sep 16, 2005
  4. ehui928

    A question about fscanf and feof !

    ehui928, Oct 15, 2006, in forum: C Programming
    Replies:
    7
    Views:
    3,146
    Joe Wright
    Nov 7, 2006
  5. kindrain

    a problem when using int feof(FILE *fp);

    kindrain, Jul 12, 2008, in forum: C Programming
    Replies:
    22
    Views:
    1,024
Loading...

Share This Page