fgets problem

[Eric Sosman]

Hi all,
I've been trying to read a text file into an array without much
success. The following is part of "main" and all appears OK until the
statement while (fgets(str,20,fp) !=NULL)
Sometimes it works fine and the file xtl.dat (argv[1]) is read in
perfectly and the program performs as expected. But more often than not,
it appears to treat the first line of xtl.dat (which is "0.51400) as
NULL and skips the rest of the file.
Can anyone see what I've done wrong?
Thanks
Alan

#include "stdio.h"

#include <math.h>
#include <stdlib.h>

float f[140] ;

main(argc,argv)
int argc ;
char *argv[] ;

Not the source of your problem, but this is very old-
fashioned, almost two decades out of date. Replace all
three lines with

int main(int argc, char *argv[])

.... to use modern style (where "modern" means "more recent
than the Reagan administration").

{
FILE *fopen() , *fp ;
float freq=20.0,tol=0.0005 ;
float freqin(),tolin() ;
char *str,c ;

Here's the beginning of your problem, although the problem
doesn't occur quite yet. `str' is a variable that can point
to a char, and you plan to use it to point to the first of a
bunch of chars. But what does it point at NOW? You haven't
given it a value, so its value is indeterminate. Informally,
it "points to nowhere," which is a perfectly acceptable state
of affairs at the moment, but ...

int i=0,j,k,size=120 ;
if ((fp=fopen(argv[1],"r")) == NULL) {
printf("No crystal list file %s\n",argv[1]) ; exit(1) ;
}
while (fgets(str,20,fp) != NULL) {

.... but now fgets() will try to store as many as 20 characters
at and after the place `str' points to. Where does `str' point?
See above, and see Question 7.2 in the comp.lang.c Frequently

f=atof(str) ;

Also not the source of your problem, but strtod() is more
robust than atof(), because it lets you discover the error
when the input looks like "1.2.3" or "x44".

for (i=0;i<130;i++) {
fgets(str,20,fp) ;

Same problem as above: where does `str' point?

f = atof(str) ;

Note that the first time through the loop you will wipe
out the value that was previously stored in f[0]. Is that
what you meant to do? If so, why did you bother storing
the earlier f[0] value?

printf(" %10s %3.6f %d",str,f,i) ;
}
fclose(fp) ;

 

[Keith Thompson]

I've been trying to read a text file into an array without much
success. The following is part of "main" and all appears OK until the
statement while (fgets(str,20,fp) !=NULL)
Sometimes it works fine and the file xtl.dat (argv[1]) is read in
perfectly and the program performs as expected. But more often than
not, it appears to treat the first line of xtl.dat (which is "0.51400)
as NULL and skips the rest of the file.
Can anyone see what I've done wrong?
Thanks
Alan

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

float f[140] ;

main(argc,argv)
int argc ;
char *argv[] ;
{
FILE *fopen() , *fp ;
float freq=20.0,tol=0.0005 ;
float freqin(),tolin() ;
char *str,c ;
int i=0,j,k,size=120 ;
if ((fp=fopen(argv[1],"r")) == NULL) {
printf("No crystal list file %s\n",argv[1]) ; exit(1) ;
}
while (fgets(str,20,fp) != NULL) {
f=atof(str) ;
for (i=0;i<130;i++) {
fgets(str,20,fp) ;
f = atof(str) ;
printf(" %10s %3.6f %d",str,f,i) ;
}
fclose(fp) ;

The code you posted is incomplete; you're missing closing braces for
the while loop and for the main function. Judging by the indentation,
you call fopen() exactly once, but you call fclose() on each iteration
of the while loop. Calling fgets() on a closed file invokes undefined
behavior. I suspect that's the cause of your problem, but it's hard
to be sure without seeing your input.

There are a number of other things you should fix; some of them are
errors, some are style issues.

Include directives for standard headers should use <>, not "".
Change
#include "stdio.h"
to
#include <stdio.h>

You don't use anything from <math.h>, so that header isn't necessary.
This might be an artifact of the way you've trimmed the program for
posting (thank you for that).

Your array "f" is used only inside main, so there's no reason to
declare it at file scope. Again, this might be an artifact of the way
you've trimmed the program for posting, but it might still make more
sense to declare it in one function and pass a pointer to other
functions that use it.

I personally dislike putting a space in front of each semicolon, and I
always put a space after each comma. Not all C programmers share this
particular opinion, but I think most do.

Your declaration of main is an old-style declaration; there's no
longer any good reason to use such declarations. Use:
int main(int argc, char *argv[])
(I personally prefer "char **argv", but "char *argv[]" is certainly
valid.)

There's no need to re-declare fopen, and in fact it's a bad idea to do
so; <stdio.h> does that for you.

I find code that declares one thing per line to be clearer:
float freq = 20.0;
float tol = 0.0005;

It usually makes more sense to use double rather than float; it
usually provides more precision and range, and on many CPUs it's just
as fast.

You declare freqin() and tolin(), but you don't define or use them.
If they exist in your actual code, it would be better to provide
prototypes at file scope, probably in a header file. It's legal to
have function declarations inside a function definition, but it's
generally a bad idea.

exit(1) is, strictly speaking, not portable; use exit(EXIT_FAILURE)
unless you're sure that you specifically want to pass a status of 1 to
the calling environment.

130 is a "magic number"; there's nothing in your program that explains
why it has that value. Declare a constant, perhaps using #define.

In the loop, you call fgets() without checking the result. This is a
bad idea; you have no way of detecting failure. This makes your
program "brittle" in the presence of bad input data. You should also
think about what to do if an input line is longer than what your array
can hold.

Don't use the atof() function; it doesn't detect errors, and in fact
can invoke undefined behavior in some cases. (The exact circumstances
in which the behavior is undefined are not entirely clear, which is
another good reason not to use it.) Use strtod() instead; it's a
little more complicated, but the extra safety is well worth i.

All your output, other than the first line, is on a single line with
no '\n' characters. Was that your intent? Strictly speaking, if your
implementation requires a terminating '\n' at the end of the last line
of output, then failing to provide one can invoke undefined behavior
(though on most systems you'll just get a long line with no
end-of-line marker at the end) Perhaps you wanted:
printf(" %10s %3.6f %d\n", str, f, i);

 

[Keith Thompson]

Alan Peake wrote: [...]

{
FILE *fopen() , *fp ;
float freq=20.0,tol=0.0005 ;
float freqin(),tolin() ;
char *str,c ;

Here's the beginning of your problem, although the problem
doesn't occur quite yet. `str' is a variable that can point
to a char, and you plan to use it to point to the first of a
bunch of chars. But what does it point at NOW? You haven't
given it a value, so its value is indeterminate. Informally,
it "points to nowhere," which is a perfectly acceptable state
of affairs at the moment, but ...

int i=0,j,k,size=120 ;
if ((fp=fopen(argv[1],"r")) == NULL) {
printf("No crystal list file %s\n",argv[1]) ; exit(1) ;
}
while (fgets(str,20,fp) != NULL) {

... but now fgets() will try to store as many as 20 characters
at and after the place `str' points to. Where does `str' point?

[...]

Argh, how did I miss that?

 

[Eric Sosman]

[...]

[...]
you call fopen() exactly once, but you call fclose() on each iteration
of the while loop. [...]

Argh, how did I miss that?

 

[CBFalconer]

I've been trying to read a text file into an array without much
success. The following is part of "main" and all appears OK until
the statement while (fgets(str,20,fp) !=NULL)

Sometimes it works fine and the file xtl.dat (argv[1]) is read in
perfectly and the program performs as expected. But more often
than not, it appears to treat the first line of xtl.dat (which is
"0.51400) as NULL and skips the rest of the file.
Can anyone see what I've done wrong?

See the description for cases where a NULL is returned:

7.19.7.2 The fgets function

Synopsis
[#1]
#include <stdio.h>
char *fgets(char * restrict s, int n,
FILE * restrict stream);

Description

[#2] The fgets function reads at most one less than the
number of characters specified by n from the stream pointed
to by stream into the array pointed to by s. No additional
characters are read after a new-line character (which is
retained) or after end-of-file. A null character is written
immediately after the last character read into the array.

Returns

[#3] The fgets function returns s if successful. If end-of-
file is encountered and no characters have been read into
the array, the contents of the array remain unchanged and a
null pointer is returned. If a read error occurs during the
operation, the array contents are indeterminate and a null
pointer is returned.

 

[vippstar]

I've been trying to read a text file into an array without much
success. The following is part of "main" and all appears OK until
the statement while (fgets(str,20,fp) !=NULL)

Sometimes it works fine and the file xtl.dat (argv[1]) is read in
perfectly and the program performs as expected. But more often
than not, it appears to treat the first line of xtl.dat (which is
"0.51400) as NULL and skips the rest of the file.
Can anyone see what I've done wrong?

See the description for cases where a NULL is returned:

The description is useless and the results meaningless if before fgets
returns from it's caller undefined behavior is invoked. Before
instructing someone to look at the description you should make sure
that this doesn't happend. You didn't make sure. It did happend. The
description is irrelevant.

I'm curious, why do you choose to read only parts of a post? I guess
this question will go unanswered since you probably stopped reading
some lines ago.

 

[CBFalconer]

I've been trying to read a text file into an array without much
success. The following is part of "main" and all appears OK until
the statement while (fgets(str,20,fp) !=NULL)

Sometimes it works fine and the file xtl.dat (argv[1]) is read in
perfectly and the program performs as expected. But more often
than not, it appears to treat the first line of xtl.dat (which is
"0.51400) as NULL and skips the rest of the file.
Can anyone see what I've done wrong?

See the description for cases where a NULL is returned:

The description is useless and the results meaningless if before
fgets returns from it's caller undefined behavior is invoked.
Before instructing someone to look at the description you should
make sure that this doesn't happend. You didn't make sure. It did
happend. The description is irrelevant.

You seem to feel that an incomplete and uncompilable program
listing is adequate data. I disagree.

 

[Barry Schwarz]

Hi all,
I've been trying to read a text file into an array without much
success. The following is part of "main" and all appears OK until the
statement while (fgets(str,20,fp) !=NULL)
Sometimes it works fine and the file xtl.dat (argv[1]) is read in
perfectly and the program performs as expected. But more often than not,
it appears to treat the first line of xtl.dat (which is "0.51400) as
NULL and skips the rest of the file.
Can anyone see what I've done wrong?
Thanks
Alan

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

float f[140] ;

main(argc,argv)
int argc ;
char *argv[] ;
{
FILE *fopen() , *fp ;

You include stdio.h. There is no reason to attempt to declare it here
also, especially since you do so incorrectly.

float freq=20.0,tol=0.0005 ;
float freqin(),tolin() ;

You don't call these functions so this line seems pointless.

char *str,c ;
int i=0,j,k,size=120 ;
if ((fp=fopen(argv[1],"r")) == NULL) {
printf("No crystal list file %s\n",argv[1]) ; exit(1) ;

1 is not a portable return value from main. Use EXIT_FAILURE which is
defined in stdlib.h which you already include.

}
while (fgets(str,20,fp) != NULL) {
f=atof(str) ;

atof does not provide any indication that it was successful. Use
strtod instead.

for (i=0;i<130;i++) {
fgets(str,20,fp) ;
f = atof(str) ;

In the first iteration of this loop, you replace the value stored in
f[0] above.

printf(" %10s %3.6f %d",str,f,i) ;
}
fclose(fp) ;

 

[vippstar]

The code you posted is incomplete; you're missing closing braces for
the while loop and for the main function.

I left out the rest of main for brevity. I've included most of yours and
other suggestions and corrections.

130 is a "magic number"; there's nothing in your program that explains
why it has that value. Declare a constant, perhaps using #define.

The file that is used in argv[] has 130 text lines and I didn't know how
to set f[] to some number in the case of a different input file with
more or less entries.
Bit rusty on things like alloc

Troll.

 

[Keith Thompson]

The code you posted is incomplete; you're missing closing braces for
the while loop and for the main function.

I left out the rest of main for brevity. I've included most of yours and
other suggestions and corrections.

130 is a "magic number"; there's nothing in your program that explains
why it has that value. Declare a constant, perhaps using #define.

The file that is used in argv[] has 130 text lines and I didn't know how
to set f[] to some number in the case of a different input file with
more or less entries.
Bit rusty on things like alloc

Troll.

Why on Earth would you assume that? He said his C is a bit rusty;
what basis do you have for accusing him of dishonesty?

 

[Keith Thompson]

I left out the rest of main for brevity. I've included most of yours
and other suggestions and corrections.

Brevity is good (the soul of wit and all that), but posting complete
compilable code is (almost) always a good idea. You were just two
closing braces short of that.

130 is a "magic number"; there's nothing in your program that
explains
why it has that value. Declare a constant, perhaps using #define.

The file that is used in argv[] has 130 text lines and I didn't know how
to set f[] to some number in the case of a different input file with
more or less entries.
Bit rusty on things like alloc

Your f array has 140 elements, and your code assumes 130 lines of
input text. I had missed your assumption that these two values were
related, and I'm guessing they were meant to be equal. That's an even
better reason to use a named constant; you can change the value of the
constant without having to change every reference to it. For example:

#define EXPECTED_LINES 140
...
float f[EXPECTED_LINES};
...
for (i = 0; i < EXPECTED_LINES; i ++)
...

If you want to use a fixed number of lines (which makes for simpler
coding), you can print an error message if you detect that the file as
more or fewer lines. If you want to allow for an arbitrary number of
lines, and you need to store information from all of them, there are
several solutions. You can declare a huge array and use only part of
it, though that's wasteful. You can expand the array as needed using
realloc(). You can use a linked list rather than an array.

Incidentally, this newsgroup has a very good FAQ at
<http://www.c-faq.com/>.

 

[vippstar]

Keith Thompson wrote:
The code you posted is incomplete; you're missing closing braces for
the while loop and for the main function.
I left out the rest of main for brevity. I've included most of yours and
other suggestions and corrections.
130 is a "magic number"; there's nothing in your program that explains
why it has that value. Declare a constant, perhaps using #define.
The file that is used in argv[] has 130 text lines and I didn't know how
to set f[] to some number in the case of a different input file with
more or less entries.
Bit rusty on things like alloc

Troll.

Why on Earth would you assume that? He said his C is a bit rusty;
what basis do you have for accusing him of dishonesty?

Maybe I'm just paranoid, but the last sentence gave him away I think.
He also doesn't have a real e-mail address. The formatting is almost
excellent (in the first post) except the semicolons.
As for including "stdio.h" instead <stdio.h>, how can you miss
something like that when the other two headers use <>? He top-posted.
He also did

char *str, c;

Then

toupper(c = getchar())

I don't think that's just coincidental. How possible could it be that
someone whose C is "rusty" to remember char *str, c; actually means
char *str; char c; and what are the chances for him to use that object
to save the getchar value returned, and then pass that to toupper?
That's so subtly wrong and right (if c is changed to int) that I
honestly think he's trying to get you all to talk about getchar &
toupper. He's another Cunningham for christ's sake!

One thing that hasn't been mentioned so far is that he's using argv[1]
in fopen but he doesn't really check if argc > 1.
He also decided to add some cls and khbit calls. (non-standard)
He snipped code for "brevity"

I just don't know how you can get so many things wrong on 3 posts (and
I only mentioned _some_ of the things he got wrong). I imagine that
he'll probably continue asking such questions and posting totally
wrong code and you'll all continue wasting your time with him. Well,
have fun, but I think I'll pass.

 

[Alan Peake]

Hi all,
I've been trying to read a text file into an array without much
success. The following is part of "main" and all appears OK until the
statement while (fgets(str,20,fp) !=NULL)
Sometimes it works fine and the file xtl.dat (argv[1]) is read in
perfectly and the program performs as expected. But more often than not,
it appears to treat the first line of xtl.dat (which is "0.51400) as
NULL and skips the rest of the file.
Can anyone see what I've done wrong?
Thanks
Alan

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

float f[140] ;

main(argc,argv)
int argc ;
char *argv[] ;
{
FILE *fopen() , *fp ;
float freq=20.0,tol=0.0005 ;
float freqin(),tolin() ;
char *str,c ;
int i=0,j,k,size=120 ;
if ((fp=fopen(argv[1],"r")) == NULL) {
printf("No crystal list file %s\n",argv[1]) ; exit(1) ;
}
while (fgets(str,20,fp) != NULL) {
f=atof(str) ;
for (i=0;i<130;i++) {
fgets(str,20,fp) ;
f = atof(str) ;
printf(" %10s %3.6f %d",str,f,i) ;
}
fclose(fp) ;

 

[Phil Carmody]

float f[140] ;

int main(int argc,char *argv[])
{
FILE *fp ;
float freq=20.0,tol=0.0005 ;
float freqin(),tolin() ;
char *str,c ;
int i=0,size ;
if ((fp=fopen(argv[1],"r")) == NULL) {
printf("No crystal list file %s\n",argv[1]) ; exit(1) ;
}
str=f ;
while (fgets(str,20,fp) != NULL) {

So you're writing those chars into your array of floats.

f = atof(str) ;

And then you're putting floats into your array of floats.

i++ ;
printf(" %10s %3.6f %d",str,f,i) ;
}

And you're looping back to write more chars into your array of floats.

size =i ;

You didn't expect the floats you wrote into that array of floats
to still be in there, did you?

If you want an array of chars into which to write, just define
an array of chars and use that. Don't try and use the same
block of memory for two different purposes at the same time;
that way all kind of unwanted troubles lie.

So scrap "char*str;" and "str=f", and use "char str[20];" instead.
For improved sanity, don't use the magic number '20' more than
once. Either use a sensibly named #define multiple times, or use
sizeof to retrieve the array's size.

Phil

 

[Alan Peake]

Many thanks Eric, that seems to be the problem. Not sure of the proper
fix but I did a "shonky" and let str=f. The compiler gave a warning
about different types of course but the program now works reliably
except that the printf(" %10s bit doesn't work. So I need to find out
how to point str a bit better
I've included your suggestions re int main but left atof until I can
work out how strtod works. Anyway, I've put all of main here for any
further comment.
Some of the previous code was a bit messy as I was trying to find the
error using CodeView. I also haven't used C 5.0 for about 10 years and
the since retirement, the old brain doesn't work quite so well
Cheers and thanks again,
Alan



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

float f[140] ;

int main(int argc,char *argv[])
{
FILE *fp ;
float freq=20.0,tol=0.0005 ;
float freqin(),tolin() ;
char *str,c ;
int i=0,size ;
if ((fp=fopen(argv[1],"r")) == NULL) {
printf("No crystal list file %s\n",argv[1]) ; exit(1) ;
}
str=f ;
while (fgets(str,20,fp) != NULL) {
f = atof(str) ;
i++ ;
printf(" %10s %3.6f %d",str,f,i) ;
}
size =i ;
while (!kbhit()) ;
fclose(fp) ;
cls() ;
do {
dca(1,1) ; printf("0 exit ") ;
dca(2,1) ; printf("1 Enter desired frequency in MHz") ;
dca(3,1) ; printf("2 Enter tolerance in MHz ") ;
dca(4,1) ; printf("3 Find crystals ") ;
dca(5,1) ; printf("4 Try all combinations ") ;
switch (toupper(c=getchar())) {
case '1' : freq = freqin() ; break ;
case '2' : tol = tolin() ; break ;
case '3' : findxt(freq,tol,size) ; break ;
case '4' : tryall(freq,tol,size) ; break ;
default : break ;
}
} while (c!='0') ;
exit(0) ;
}

Here's the beginning of your problem, although the problem
doesn't occur quite yet. `str' is a variable that can point
to a char, and you plan to use it to point to the first of a
bunch of chars. But what does it point at NOW? You haven't
given it a value, so its value is indeterminate. Informally,
it "points to nowhere," which is a perfectly acceptable state
of affairs at the moment, but ...

int i=0,j,k,size=120 ;
if ((fp=fopen(argv[1],"r")) == NULL) {
printf("No crystal list file %s\n",argv[1]) ; exit(1) ;
}
while (fgets(str,20,fp) != NULL) {

... but now fgets() will try to store as many as 20 characters
at and after the place `str' points to. Where does `str' point?

 

[Kenny McCormack]

Troll.

Why on Earth would you assume that? He said his C is a bit rusty;
what basis do you have for accusing him of honesty?[/QUOTE]

Fixed it for you. No charge.

The fact is that, in this newsgroup, the "trolls" are the only ones that
speak truth.

 

[Alan Peake]

The code you posted is incomplete; you're missing closing braces for
the while loop and for the main function.

I left out the rest of main for brevity. I've included most of yours and
other suggestions and corrections.

130 is a "magic number"; there's nothing in your program that explains
why it has that value. Declare a constant, perhaps using #define.

The file that is used in argv[] has 130 text lines and I didn't know how
to set f[] to some number in the case of a different input file with
more or less entries.
Bit rusty on things like alloc

Alan

 

[Antoninus Twink]

Maybe I'm just paranoid

Say, d'ya reckon?

I just don't know how you can get so many things wrong on 3 posts

Try asking CBF - he's a past master at getting things wrong in post
after post.

 

[Ben Bacarisse]

A few more remarks...

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

float f[140] ;

I'd name the size for later use:

#define N_FLOATS 140

float f[N_FLOATS];

and I'd use another name if I knew what these numbers were (float
angles[N_ANGLES]; for example).

int main(int argc,char *argv[])
{
FILE *fp ;
float freq=20.0,tol=0.0005 ;
float freqin(),tolin() ;
char *str,c ;
int i=0,size ;
if ((fp=fopen(argv[1],"r")) == NULL) {

You might as well add:

if (argc != 2 || (fp = fopen(argv[1], "r")) == NULL) {

That way you don't try to open a file when no name is given. You then
need to fix the message:

printf("No crystal list file %s\n",argv[1]) ; exit(1) ;

Which I'd do like this:

printf("No crystal list file %s\n", argc > 1 ? argv[1] : "name provided");

but there are lots of other less sneaky solutions.

}
str=f ;
while (fgets(str,20,fp) != NULL) {

You need a separate array (as already mentioned). After

char str[20];

you can write fgets(str, sizeof str, fp), but for simple input like
this I actually prefer the much maligned fscanf. I also prefer to
check that I can put the data somewhere, even if I don't report the
error of running out of room in a first draft of the program:

while (i < N_FLOATS && fscanf("%f", &f) == 1) {

It is a good habit to get into.

<snip>
It is now hours after I started the post so I am sure other people
have said similar things, but I'd already typed too much to abandon
posting!

 

[CBFalconer]

Many thanks Eric, that seems to be the problem. Not sure of the
proper fix but I did a "shonky" and let str=f. The compiler gave
a warning about different types of course but the program now
works reliably except that the printf(" %10s bit doesn't work.
So I need to find out how to point str a bit better

Please do not top-post. Your answer belongs after (or intermixed
with) the quoted material to which you reply, after snipping all
irrelevant material. See the following links:

<http://www.catb.org/~esr/faqs/smart-questions.html>
<http://www.caliburn.nl/topposting.html>
<http://www.netmeister.org/news/learn2quote.html>
<http://cfaj.freeshell.org/google/> (taming google)
<http://members.fortunecity.com/nnqweb/> (newusers)