Velocity Reviews - Computer Hardware Reviews

Velocity Reviews > Newsgroups > Programming > C Programming > read line function outputs weird results

Reply
Thread Tools

read line function outputs weird results

 
 
WStoreyII
Guest
Posts: n/a
 
      02-14-2007
the following code is supposed to read a whole line upto a new line
char from a file. however it does not work. it is producing weird
results. please help. I had error checking in there for mallocs and
ect, but i removed them to help me debug. thanks.

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

void freadl ( FILE *stream, char **string ) {

char next = fgetc ( stream );
char *line;

line = (char*) malloc (1*sizeof(char));

while ( next != '\n' ) {

line = (char*) realloc ( line, (strlen(line) + 1) * sizeof(char) );
line [strlen(line)] = next;
next = fgetc (stream );
}

*string = (char*) malloc((strlen(line))*sizeof(char));
strcpy (*string,line);
free(line);

}

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

FILE *fp;
fp = fopen ( "fread.c", "r" );

char *str;
freadl ( fp , &str );

printf ( "str = %s, with size of %d\n", str, strlen (str) );

int i;

for ( i = 0; i < strlen (str); i++ ){

char now = str[i];
printf ( "chr = %c, # = %d\n", now,now);

}

}

 
Reply With Quote
 
 
 
 
Ian Collins
Guest
Posts: n/a
 
      02-14-2007
WStoreyII wrote:
> the following code is supposed to read a whole line upto a new line
> char from a file. however it does not work. it is producing weird
> results. please help. I had error checking in there for mallocs and
> ect, but i removed them to help me debug. thanks.
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> void freadl ( FILE *stream, char **string ) {
>
> char next = fgetc ( stream );
> char *line;
>
> line = (char*) malloc (1*sizeof(char));
>

Why oh why are people still adding these extraneous casts? Not only
that, but sizeof(char) is, buy definition 1.

> while ( next != '\n' ) {
>
> line = (char*) realloc ( line, (strlen(line) + 1) * sizeof(char) );


You are calling strlen on an array of char that isn't null terminated.

> line [strlen(line)] = next;


You are calling strlen on an array of char that isn't null terminated.

> next = fgetc (stream );
> }
>
> *string = (char*) malloc((strlen(line))*sizeof(char));


You are calling strlen on an array of char that isn't null terminated.

--
Ian Collins.
 
Reply With Quote
 
 
 
 
Joachim Schmitz
Guest
Posts: n/a
 
      02-14-2007

"Ian Collins" <(E-Mail Removed)> schrieb im Newsbeitrag
news:(E-Mail Removed)...
> WStoreyII wrote:
>> line = (char*) malloc (1*sizeof(char));
>>

> Why oh why are people still adding these extraneous casts? Not only
> that, but sizeof(char) is, buy definition 1.

For several reasons: it is mentiond in lots of books about C and it is
required by C++
I admit that neither is a good reason (the books are just wrong and C++ is
OT here), but at least understandable. And a minor issue, there are more
important things to moan about, won't you think?

Bye, Jojo.


 
Reply With Quote
 
santosh
Guest
Posts: n/a
 
      02-14-2007
WStoreyII wrote:
> the following code is supposed to read a whole line upto a new line
> char from a file. however it does not work. it is producing weird
> results. please help. I had error checking in there for mallocs and
> ect, but i removed them to help me debug. thanks.
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> void freadl ( FILE *stream, char **string ) {
>
> char next = fgetc ( stream );


fgetc() returns an int value. This is because, it has no way of
indicating failure within the range of a char value, since it could be
a legal. Hence it returns an out-of-band value, represented
symbolically as EOF. So you should test each invocation of fgetc(),
getc(), getchar() etc. like:

int next;
if((next = fgetc(stream)) == EOF) /* handle error */

or something similar.

> char *line;
>
> line = (char*) malloc (1*sizeof(char));


The casts are not needed in C. Do:
line = malloc(1 * sizeof *line);

> while ( next != '\n' ) {
>
> line = (char*) realloc ( line, (strlen(line) + 1) * sizeof(char) );


There're multiple error here. First, if realloc() happens to fail, you
overwrite the address of the original buffer with a null pointer
value, thus loosing access to it. You should always backup the address
of the buffer before attempting to reallocating it. Secondly, as
above, the cast is once again uneccessary.

Now coming to the main mistake. You're passing a unbounded array to
strlen(). strlen() is used to find the length of C strings and you
must supply it a nul terminated array. While all strings are arrays,
all arrays are not strings, i.e. nul terminated, as in this case.

I suggest increasing the buffer by a factor of two. You must keep
track of the size of the buffer yourself, presumably with a size_t
object.

> line [strlen(line)] = next;


Same mistake. The buffer you're using is *not* storing a string.
You're just reading a sequence of characters. strlen() is the wrong
function to use. You have to keep track of the buffer length manually.

> next = fgetc (stream );
> }
>
> *string = (char*) malloc((strlen(line))*sizeof(char));


No need for an unneccessary allocation here. Dynamically allocated
objects persist until they're free()'ed or until program termination.
Hence, all you need to do is to pass a pointer to the actual buffer
back to the calling function, along with the size of the line read
into the buffer.

> strcpy (*string,line);
> free(line);
>
> }
>
> int main ( int argc, char *argv[] ) {
>
> FILE *fp;
> fp = fopen ( "fread.c", "r" );


Check the return values of standard library functions. Just going
ahead assuming success is asking for difficult to find bugs.

> char *str;
> freadl ( fp , &str );
>
> printf ( "str = %s, with size of %d\n", str, strlen (str) );
>
> int i;
>
> for ( i = 0; i < strlen (str); i++ ){
>
> char now = str[i];
> printf ( "chr = %c, # = %d\n", now,now);
>
> }
>
> }


The main mistake you're making is assuming that you can find out the
length of any object with strlen(). The latter only works on objects
terminated by a nul character, '\0'. For other objects, you'll have to
track their length by other methods.

 
Reply With Quote
 
pete
Guest
Posts: n/a
 
      02-14-2007
WStoreyII wrote:
>
> the following code is supposed to read a whole line upto a new line
> char from a file. however it does not work. it is producing weird
> results. please help. I had error checking in there for mallocs and
> ect, but i removed them to help me debug. thanks.
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> void freadl ( FILE *stream, char **string ) {
>
> char next = fgetc ( stream );
> char *line;
>
> line = (char*) malloc (1*sizeof(char));
>
> while ( next != '\n' ) {
>
> line = (char*) realloc ( line, (strlen(line) + 1) * sizeof(char) );
> line [strlen(line)] = next;
> next = fgetc (stream );
> }
>
> *string = (char*) malloc((strlen(line))*sizeof(char));
> strcpy (*string,line);
> free(line);
>
> }
>
> int main ( int argc, char *argv[] ) {
>
> FILE *fp;
> fp = fopen ( "fread.c", "r" );
>
> char *str;
> freadl ( fp , &str );
>
> printf ( "str = %s, with size of %d\n", str, strlen (str) );
>
> int i;
>
> for ( i = 0; i < strlen (str); i++ ){
>
> char now = str[i];
> printf ( "chr = %c, # = %d\n", now,now);
>
> }
>
> }


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

void freadl(FILE *stream, char **string)
{
unsigned long counter = 0;
char *line = NULL;
int next = fgetc(stream);

do {
next = fgetc(stream);
if (next == EOF) {
free(line);
break;
}
++counter;
line = realloc(line, counter + 1);
if (line == NULL) {
puts("line == NULL");
exit(EXIT_FAILURE);
}
line[counter - 1] = (char)next;
} while (next != '\n');
line[counter - 1] = '\0';
*string = malloc(strlen(line) + 1);
if (*string == NULL) {
puts("*string == NULL");
} else {
strcpy(*string, line);
}
free(line);
}

int main (void)
{
char *str;
FILE *fp = fopen("fread.c", "r");

if (fp != NULL) {
freadl(fp , &str);
printf("str = %s, with length of %u\n",
str, (unsigned)strlen(str));
} else {
puts("fp == NULL");
}
return 0;
}
--
pete
 
Reply With Quote
 
WStoreyII
Guest
Posts: n/a
 
      02-14-2007
On Feb 14, 5:06 am, pete <(E-Mail Removed)> wrote:
> WStoreyII wrote:
>
> > the following code is supposed to read a whole line upto a new line
> > char from a file. however it does not work. it is producing weird
> > results. please help. I had error checking in there for mallocs and
> > ect, but i removed them to help me debug. thanks.

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

>
> > void freadl ( FILE *stream, char **string ) {

>
> > char next = fgetc ( stream );
> > char *line;

>
> > line = (char*) malloc (1*sizeof(char));

>
> > while ( next != '\n' ) {

>
> > line = (char*) realloc ( line, (strlen(line) + 1) * sizeof(char) );
> > line [strlen(line)] = next;
> > next = fgetc (stream );
> > }

>
> > *string = (char*) malloc((strlen(line))*sizeof(char));
> > strcpy (*string,line);
> > free(line);

>
> > }

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

>
> > FILE *fp;
> > fp = fopen ( "fread.c", "r" );

>
> > char *str;
> > freadl ( fp , &str );

>
> > printf ( "str = %s, with size of %d\n", str, strlen (str) );

>
> > int i;

>
> > for ( i = 0; i < strlen (str); i++ ){

>
> > char now = str[i];
> > printf ( "chr = %c, # = %d\n", now,now);

>
> > }

>
> > }

>
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
>
> void freadl(FILE *stream, char **string)
> {
> unsigned long counter = 0;
> char *line = NULL;
> int next = fgetc(stream);
>
> do {
> next = fgetc(stream);
> if (next == EOF) {
> free(line);
> break;
> }
> ++counter;
> line = realloc(line, counter + 1);
> if (line == NULL) {
> puts("line == NULL");
> exit(EXIT_FAILURE);
> }
> line[counter - 1] = (char)next;
> } while (next != '\n');
> line[counter - 1] = '\0';
> *string = malloc(strlen(line) + 1);
> if (*string == NULL) {
> puts("*string == NULL");
> } else {
> strcpy(*string, line);
> }
> free(line);
>
> }
>
> int main (void)
> {
> char *str;
> FILE *fp = fopen("fread.c", "r");
>
> if (fp != NULL) {
> freadl(fp , &str);
> printf("str = %s, with length of %u\n",
> str, (unsigned)strlen(str));
> } else {
> puts("fp == NULL");
> }
> return 0;}
>
> --
> pete



thanks for all of the help. i will try these out later. as far as
the cast goes and using sizeof(char). i am new to c (obviously) and
everything that i have read so far has said to use these methods. so
as with anything there are those who are strict and those who are
not. i have actually got compiler warnings though for not having the
casts on malloc.

thanks again for your help.

 
Reply With Quote
 
Christopher Layne
Guest
Posts: n/a
 
      02-14-2007
WStoreyII wrote:

> thanks for all of the help. i will try these out later. as far as
> the cast goes and using sizeof(char). i am new to c (obviously) and
> everything that i have read so far has said to use these methods. so
> as with anything there are those who are strict and those who are
> not. i have actually got compiler warnings though for not having the
> casts on malloc.
>
> thanks again for your help.


BTW: Is this for an intended purpose? If not, you can just use fgets().
 
Reply With Quote
 
Richard Heathfield
Guest
Posts: n/a
 
      02-14-2007
WStoreyII said:

> i have actually got compiler warnings though for not having the
> casts on malloc.


That is almost certainly caused by one of two problems:

a) you forgot to #include <stdlib.h>

b) you're using a C++ compiler, not a C compiler

If it's a), then add the include. If it's b), you have two reasonable
courses of action: i) use a C compiler instead, or ii) write in C++
instead.

If you choose to write in C++ instead, you should probably consider
malloc to be obsolete, and use new, new[], or std::vector instead. If
you choose the C++ route, please direct further discussions thereof to
comp.lang.c++ rather than this newsgroup.

--
Richard Heathfield
"Usenet is a strange place" - dmr 29/7/1999
http://www.cpax.org.uk
email: rjh at the above domain, - www.
 
Reply With Quote
 
pete
Guest
Posts: n/a
 
      02-14-2007
WStoreyII wrote:

> i have actually got compiler warnings though for not having the
> casts on malloc.


The typical cause of that warning is the failure to write
#include <stdlib.h>
prior to the call to malloc.
The compiler then assumes that malloc returns type int,
and then the warning is about attempting
to assign an integer type value to a pointer.

That's why casting the return value of malloc is bad.
It tends to conceal the real error,
which is the failure to include stdlib.h.

--
pete
 
Reply With Quote
 
Flash Gordon
Guest
Posts: n/a
 
      02-14-2007
Joachim Schmitz wrote, On 14/02/07 09:45:
> "Ian Collins" <(E-Mail Removed)> schrieb im Newsbeitrag
> news:(E-Mail Removed)...
>> WStoreyII wrote:
>>> line = (char*) malloc (1*sizeof(char));
>>>

>> Why oh why are people still adding these extraneous casts? Not only
>> that, but sizeof(char) is, buy definition 1.

> For several reasons: it is mentiond in lots of books about C and it is
> required by C++
> I admit that neither is a good reason (the books are just wrong and C++ is
> OT here), but at least understandable. And a minor issue, there are more
> important things to moan about, won't you think?


As shown by a later reply of WStoreyII's the cast was actually hiding
one of a couple of serious errors. The errors being either using a
compiler in C++ mode for C or failing to include stdlib.h when needed.
So the comment about the cast was important and has shown up a serious
error. It is because of the hiding of serious errors that most people
here recommend against casting the result of malloc (or any other cast
that is not actually required).
--
Flash Gordon
 
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
Problem with displaying command line outputs John Smith Ruby 2 04-20-2011 02:47 PM
Read a file line by line and write each line to a file based on the5th byte scad C++ 23 05-17-2009 06:11 PM
How to read a text file line by line and remove some line kaushikshome C++ 4 09-10-2006 10:12 PM
Beginner: read $array with line breaks line by line Marek Stepanek Perl Misc 12 09-02-2006 10:27 AM
Read a file line by line with a maximum number of characters per line Hugo Java 10 10-18-2004 11:42 AM



Advertisments