problem with feof ?

[mark.babli]

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

 

[Joe Pfeiffer]

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?

 

[Andrew Smallshaw]

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.

 

[Ben Bacarisse]

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. */

 

[Barry Schwarz]

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).

 

[James Kuyper]

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".

 

[glen herrmannsfeldt]

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)


(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

 

[Shao Miller]

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'

 

[Keith Thompson]

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.

 

[mark.babli]

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 "

 

[mark.babli]

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.

 

[Ian Collins]

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?

 

[mark.babli]

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?

 

[mark.babli]

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.

 

[mark.babli]

I did apply all suggestions and will carry them in my code moving forward.
Thanks

 

[Barry Schwarz]

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.

 

[Barry Schwarz]

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?

 

[mark.babli]

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.

 

[Willem]

(e-mail address removed) 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

 

[Tim Rentsch]

[..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.