Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > fgets problem

Reply
Thread Tools

fgets problem

 
 
Eric Sosman
Guest
Posts: n/a
 
      12-22-2008
Alan Peake wrote:
> 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"


Should be <stdio.h>, with angle brackets.

> #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
Asked Questions (FAQ) list at <http://www.c-faq.com/>.

> f[i]=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[i] = 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],i) ;
> }
> fclose(fp) ;
>


--
Eric Sosman
http://www.velocityreviews.com/forums/(E-Mail Removed)lid
 
Reply With Quote
 
 
 
 
Keith Thompson
Guest
Posts: n/a
 
      12-22-2008
Alan Peake <(E-Mail Removed)> writes:
> 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[i]=atof(str) ;
> for (i=0;i<130;i++) {
> fgets(str,20,fp) ;
> f[i] = atof(str) ;
> printf(" %10s %3.6f %d",str,f[i],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], i);

--
Keith Thompson (The_Other_Keith) (E-Mail Removed) <http://www.ghoti.net/~kst>
Nokia
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"
 
Reply With Quote
 
 
 
 
Keith Thompson
Guest
Posts: n/a
 
      12-22-2008
Eric Sosman <(E-Mail Removed)> writes:
> 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?

--
Keith Thompson (The_Other_Keith) (E-Mail Removed) <http://www.ghoti.net/~kst>
Nokia
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"
 
Reply With Quote
 
Eric Sosman
Guest
Posts: n/a
 
      12-22-2008
Keith Thompson wrote:
> Alan Peake <(E-Mail Removed)> writes:
>> [...]

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


Argh, how did I miss that?

--
Eric Sosman
(E-Mail Removed)lid
 
Reply With Quote
 
CBFalconer
Guest
Posts: n/a
 
      12-23-2008
Alan Peake wrote:
>
> 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.

--
[mail]: Chuck F (cbfalconer at maineline dot net)
[page]: <http://cbfalconer.home.att.net>
Try the download section.
 
Reply With Quote
 
vippstar@gmail.com
Guest
Posts: n/a
 
      12-23-2008
On Dec 23, 2:26 am, CBFalconer <(E-Mail Removed)> wrote:
> Alan Peake wrote:
>
> > 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.
 
Reply With Quote
 
CBFalconer
Guest
Posts: n/a
 
      12-23-2008
(E-Mail Removed) wrote:
> CBFalconer <(E-Mail Removed)> wrote:
>> Alan Peake wrote:
>>
>>> 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.

--
[mail]: Chuck F (cbfalconer at maineline dot net)
[page]: <http://cbfalconer.home.att.net>
Try the download section.
 
Reply With Quote
 
Barry Schwarz
Guest
Posts: n/a
 
      12-23-2008
On Tue, 23 Dec 2008 08:37:59 +0000, Alan Peake
<(E-Mail Removed)> wrote:

>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[i]=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[i] = 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],i) ;
> }
> fclose(fp) ;


--
Remove del for email
 
Reply With Quote
 
vippstar@gmail.com
Guest
Posts: n/a
 
      12-23-2008
On Dec 23, 3:44 pm, Alan Peake <(E-Mail Removed)>
wrote:
> 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.
 
Reply With Quote
 
Keith Thompson
Guest
Posts: n/a
 
      12-23-2008
(E-Mail Removed) writes:
> On Dec 23, 3:44 pm, Alan Peake <(E-Mail Removed)>
> wrote:
>> 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?

--
Keith Thompson (The_Other_Keith) (E-Mail Removed) <http://www.ghoti.net/~kst>
Nokia
"We must do something. This is something. Therefore, we must do this."
-- Antony Jay and Jonathan Lynn, "Yes Minister"
 
Reply With Quote
 
 
 
Reply

Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Trackbacks are On
Pingbacks are On
Refbacks are Off


Similar Threads
Thread Thread Starter Forum Replies Last Post
fwrite, fgets binary file readback problem Jeff C Programming 16 06-09-2008 12:03 PM
Replacing fgets Problem solved. FireHead C Programming 3 04-14-2008 03:46 AM
Problem with fgets reading last line over and over Trond Valen C++ 5 12-07-2005 08:23 AM
problem with fgets Emerson C Programming 7 12-31-2004 04:24 AM
Newbie fgets problem g.managoli@gmail.com C Programming 8 11-02-2004 08:17 PM



Advertisments