code

[Bill Cunningham]

I thought I would post this code. It seems to do what I want it to but I
thought I would have it critiqued. I use C89 but I think that maybe some of
the code maybe misplaced. For example, the fopen probably should be under
the strtod's shouldn't it ?

/*code C89 */
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[])
{
if (argc != 6) {
puts("print usage error");
exit(EXIT_FAILURE);
}
FILE *fp;
char *error;
double a, b, c, d;
if ((fp = fopen(argv[5], "a")) == NULL)
perror(error);
a = strtod(argv[1], NULL);
b = strtod(argv[2], NULL);
c = strtod(argv[3], NULL);
d = strtod(argv[4], NULL);
fprintf(fp, "%.2f\t%.2f\t%.2f\t%.2f\n", a, b, c, d);
if ((fclose(fp)) == EOF)
perror(error);
return 0;
}

Thanks all.

Bill

 

[Andrew Poelstra]

I thought I would post this code. It seems to do what I want it to but I
thought I would have it critiqued. I use C89 but I think that maybe some of
the code maybe misplaced. For example, the fopen probably should be under
the strtod's shouldn't it ?

/*code C89 */
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[])
{
if (argc != 6) {
puts("print usage error");
exit(EXIT_FAILURE);
}

Well, at this point you're mixing declarations and code. Move
this if statement below your declarations.

FILE *fp;
char *error;

error needs to point somewhere before you use it - what you do
here invokes undefined behavior because the value of error is
indeterminate.

double a, b, c, d;
if ((fp = fopen(argv[5], "a")) == NULL)
perror(error);
a = strtod(argv[1], NULL);
b = strtod(argv[2], NULL);
c = strtod(argv[3], NULL);
d = strtod(argv[4], NULL);

It might also do to check if all those strtod() calls were
successful.

 

[Keith Thompson]

I thought I would post this code. It seems to do what I want it to but I
thought I would have it critiqued. I use C89 but I think that maybe some of
the code maybe misplaced. For example, the fopen probably should be under
the strtod's shouldn't it ?

/*code C89 */
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[])
{
if (argc != 6) {
puts("print usage error");
exit(EXIT_FAILURE);
}
FILE *fp;
char *error;
double a, b, c, d;
if ((fp = fopen(argv[5], "a")) == NULL)
perror(error);
a = strtod(argv[1], NULL);
b = strtod(argv[2], NULL);
c = strtod(argv[3], NULL);
d = strtod(argv[4], NULL);
fprintf(fp, "%.2f\t%.2f\t%.2f\t%.2f\n", a, b, c, d);
if ((fclose(fp)) == EOF)
perror(error);
return 0;
}

A better usage error than "print usage error" would be nice. Telling
us what it's supposed to do would be even nicer. The intent *seems*
obvious, but we can't be sure what you're trying to do.

Appending the output to the named file seems like an odd choice. You
can certainly do that if you like, but that should be mentioned in
your explanation of what the program is supposed to do.

As written, I don't see that the ordering of the fopen and strtod
calls makes any difference. If you were doing proper error checking,
it could affect the behavior; in that case you should decide which
kind of error you want to detect first. You could do it either way.
If you're thinking that fopen should follow strtod because of C89's
requirements on ordering of declarations and statements, you're wrong;
both are statements.

"error" is never initialized, but you pass its value to perror twice.
If you're on a Unix-like system, try passing "/dev/null/nosuchfile" as
the 5th argument; if you're on a Windows-like system, try something
like "foo?bar" (Windows forbids '?' in file names).

If fopen() fails, you print an error message *and then continue
executing as if nothing had happened*.

You don't check for errors in strtod(). The second parameter to
strtod is there for a reason. Try running your program with arguments
"one two three four output.txt". Consult the documentation for
strtod() to understand what you see in output.txt.

 

[Keith Thompson]

/*code C89 */
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[])
{
if (argc != 6) {
puts("print usage error");
exit(EXIT_FAILURE);
}
FILE *fp;
char *error;
double a, b, c, d;
if ((fp = fopen(argv[5], "a")) == NULL)
perror(error);
a = strtod(argv[1], NULL);
b = strtod(argv[2], NULL);
c = strtod(argv[3], NULL);
d = strtod(argv[4], NULL);
fprintf(fp, "%.2f\t%.2f\t%.2f\t%.2f\n", a, b, c, d);
if ((fclose(fp)) == EOF)
perror(error);
return 0;
}

The program has no instances of undefined behavior, although the
perror() calls are not guaranteed to output useful information.

Yes, it does; it uses the value of error without having assigned
anything to it.

The fopen() will fail if the file does not already exist. If you
wanted to create the file if it does not already exist, or append to
it if it does, you could use the mode "a+".

I'm afraid you're mistaken on both counts. fopen() with mode "a" will
create the file if it doesn't already exist; "a+" is like "a" except
that it allows you to both read and write. C99 7.19.5.3p3:

a append; open or create text file for writing at end-of-file
[...]
a+ append; open or create text file for update, writing at end-of-file

[...]

If you were going to check the validity of those four inputs, and not
write anything to the output file unless all four were valid, then it
would make sense to put the fopen() after them, so you could skip it
if one or more of those inputs were bad.

Agreed (in contrast to my previous response). It makes sense to do
error checks that can be done in memory before doing checks that
require interactions with the file system. There's no point in
creating the output file or updating its timestamp if you're not going
to be writing anything to it because of another error.

That's assuming you're going to abort on the first error you find; if
you want to report all possible errors (can't create file *and* bad
floating-point number), the order may not matter much. But for Bill's
purposes, I'd suggest just aborting with an error message on the first
detected error.

 

[Bill Cunningham]

"error" is never initialized, but you pass its value to perror twice.
If you're on a Unix-like system, try passing "/dev/null/nosuchfile" as
the 5th argument; if you're on a Windows-like system, try something
like "foo?bar" (Windows forbids '?' in file names).

If fopen() fails, you print an error message *and then continue
executing as if nothing had happened*.

You don't check for errors in strtod(). The second parameter to
strtod is there for a reason. Try running your program with arguments
"one two three four output.txt". Consult the documentation for
strtod() to understand what you see in output.txt.

I'm using gcc-3.4.6 on linux. I might use mingw on my XP rarely but I
like the *nix enviornment better with C. C seems more at home on a *nix.

I have seen
char *error; or pointers to types before. How would this be initialized
maybe as such
char *error='\0';

It's my understanding that gcc is both C89 and C99. It seems to be
versatile. Several have pointed out the oddities of char *error.

Bill

 

[Bill Cunningham]

if ((fp = fopen(argv[5], "a")) == NULL)
perror(error);

The line above is not portable, fopen() is not required to set errno
on failure. If it was me, I would have just written:

fprintf(stderr, "Can't open %s\n", argv[5]);

[snip]

So using perror with fopen is not a good idea? That sounds to me like
what you're saying. This is what I usually type becuase I actually don't
much use perror of feof either.

if ((fp=fopen(argv[5],"a"))==NULL) {
puts("fopen failure");
exit(EXIT_FAILURE);
}

Something like that is portable isn't it? That's my normal modus operandi.

Bill

 

[Keith Thompson]

I'm using gcc-3.4.6 on linux. I might use mingw on my XP rarely but I
like the *nix enviornment better with C. C seems more at home on a *nix.

What does that have to do with anything? What I wrote above has
nothing to do with which platform you're using.

I have seen
char *error; or pointers to types before. How would this be initialized
maybe as such
char *error='\0';

You're guessing. Stop it.

You declared "error" as a pointer object. '\0' is a character value.
Why would you even *consider* initializing a pointer object to a
character value? And what is the purpose of "error"?

Or are you, as many of us suspect, deliberately trolling? (And
*please* let's not all jump in yet again with our opinions on whether
Bill is a troll; it's all been said before.)

It's my understanding that gcc is both C89 and C99. It seems to be
versatile. Several have pointed out the oddities of char *error.

Again, I haven't a clue how this relates to what I wrote. (You're not
confusing "error" with "errno", are you?)

You are calling the strtod function without checking for errors. Read
the documentation for strtod ("man strtod", or look it up in K&R2) to
find out how to check for errors. K&R2 probably has some good
examples.

 

[Bill Cunningham]

[snip]

You declared "error" as a pointer object. '\0' is a character value.
Why would you even *consider* initializing a pointer object to a
character value? And what is the purpose of "error"?

Or are you, as many of us suspect, deliberately trolling? (And
*please* let's not all jump in yet again with our opinions on whether
Bill is a troll; it's all been said before.)


Again, I haven't a clue how this relates to what I wrote. (You're not
confusing "error" with "errno", are you?)

You are calling the strtod function without checking for errors. Read
the documentation for strtod ("man strtod", or look it up in K&R2) to
find out how to check for errors. K&R2 probably has some good
examples.

I noticed a char* is passed to perror. I *assumed* that a char * needed
to be declared to pass a pointer to perror. perror is a function I rarely
use and don't know much about. I usually write my own error messages.

I pass argv[5] as a file name to fopen because on the command line is
where the filename is written to pass to open. The program is executed like
this.

p 3.4 4 5 4.5 filename

filename can be a file that exists or needs to be written. The program
is intended to write to a text file securities amounts in USD.

I thought you might have inquired as to the compiler I was using. To see
if it was C89 or C99.

Bill

 

[Bill Cunningham]

[snip]

The original code is perfectly portable (except for the fact
that error was uninitialized), it merely produces a less
than ideal error message on systems where fopen() does not
set errno. To get the filename in the error message,
Bill can do:

if( (fp = fopen( filename = argv[ 5 ], "a" )) == NULL ) {
perror( filename );
exit( EXIT_FAILURE );
}

I'm a little confused as to perror (filename); I declared char *error to
pass to perror. Again though perror is something I rarely use.

Bill

 

[vippstar]

"Bill Cunningham" <[email protected]> writes:

You're guessing. Stop it.

You declared "error" as a pointer object. '\0' is a character value.
Why would you even *consider* initializing a pointer object to a
character value? And what is the purpose of "error"?

Even though I believe you know this, I'd like to clarify because this
might confuse some.

'\0' is an integer constant, with type int in C. (however, in that
other language, C++, its type is char)
char *error = '\0'; is correct because whenever an integer constant
with value 0 is encountered in pointer context it becomes a null
pointer constant.

Question 5.9 of the C-FAQ might be of help to the interested ones.
<http://c-faq.com/>

 

[Barry Schwarz]

I'm using gcc-3.4.6 on linux. I might use mingw on my XP rarely but I
like the *nix enviornment better with C. C seems more at home on a *nix.

I have seen
char *error; or pointers to types before. How would this be initialized
maybe as such
char *error='\0';

The dismaying thing is this won't generate a diagnostic and you will
spend the next several messages arguing that it is correct. Try
looking up the NULL macro in your reference. And then ask yourself
what happens if you pass a pointer to perror that does not point to a
string.

 

[Barry Schwarz]

if ((fp = fopen(argv[5], "a")) == NULL)
perror(error);

The line above is not portable, fopen() is not required to set errno
on failure. If it was me, I would have just written:

fprintf(stderr, "Can't open %s\n", argv[5]);

[snip]

So using perror with fopen is not a good idea? That sounds to me like
what you're saying. This is what I usually type becuase I actually don't
much use perror of feof either.

if ((fp=fopen(argv[5],"a"))==NULL) {
puts("fopen failure");

It would be nice if you told the poor user which file failed to open.

exit(EXIT_FAILURE);
}

Something like that is portable isn't it? That's my normal modus operandi.

Only with the appropriate includes and declarations.

 

[Bill Cunningham]

The dismaying thing is this won't generate a diagnostic and you will
spend the next several messages arguing that it is correct.

I don't think so.

Try

looking up the NULL macro in your reference. And then ask yourself
what happens if you pass a pointer to perror that does not point to a
string.

It won't work that's for sure.

Bill

 

[Barry Schwarz]

[snip]

The original code is perfectly portable (except for the fact
that error was uninitialized), it merely produces a less
than ideal error message on systems where fopen() does not
set errno. To get the filename in the error message,
Bill can do:

if( (fp = fopen( filename = argv[ 5 ], "a" )) == NULL ) {
perror( filename );
exit( EXIT_FAILURE );
}

I'm a little confused as to perror (filename); I declared char *error to
pass to perror. Again though perror is something I rarely use.

Then what prompted you to use it this time?

 

[Barry Schwarz]

[snip]

You declared "error" as a pointer object. '\0' is a character value.
Why would you even *consider* initializing a pointer object to a
character value? And what is the purpose of "error"?

Or are you, as many of us suspect, deliberately trolling? (And
*please* let's not all jump in yet again with our opinions on whether
Bill is a troll; it's all been said before.)


Again, I haven't a clue how this relates to what I wrote. (You're not
confusing "error" with "errno", are you?)

You are calling the strtod function without checking for errors. Read
the documentation for strtod ("man strtod", or look it up in K&R2) to
find out how to check for errors. K&R2 probably has some good
examples.

I noticed a char* is passed to perror. I *assumed* that a char * needed

Did you not look up what perror does? On what did you base your
assumption? After all the previous discussions advising against it,
you are still just guessing.

to be declared to pass a pointer to perror. perror is a function I rarely
use and don't know much about. I usually write my own error messages.

I pass argv[5] as a file name to fopen because on the command line is
where the filename is written to pass to open. The program is executed like
this.

p 3.4 4 5 4.5 filename

filename can be a file that exists or needs to be written. The program
is intended to write to a text file securities amounts in USD.

I thought you might have inquired as to the compiler I was using. To see
if it was C89 or C99.

Bill

 

[Barry Schwarz]

I don't think so.

Try

It won't work that's for sure.

Then why did you suggest it as a solution when people commented on
your unassigned pointer?

 

[Bill Cunningham]

[snip]

Did you not look up what perror does? On what did you base your
assumption? After all the previous discussions advising against it,
you are still just guessing.

This comes from a C reference I have. It is not kandr2 but it helps.

"After a system function call has resulted in an error, you can use perror()
to write the string pointed to by string to stderr, followed by a colon and
the appropriate system error message."

That really doesn't help me much. Not with perror. void perror(const
char* string) is the parameter of course. I assumed I had to pass a string
to perror. Hince the char *error string. I know I've done something wrong
here but I must admit, my lack of use and understanding of perror does have
me a little dumbfounded. I have used perror successfully before but it's
been so long. As I say I don't use it much.

Bill

 

[Bill Cunningham]

Then why did you suggest it as a solution when people commented on
your unassigned pointer?

I didn't know my pointer was unassigned. I passed it to perror.

Bill

 

[Bill Cunningham]

Of course it's not helping when it comes to perror.

Bill

 

[Keith Thompson]

[snip]

Did you not look up what perror does? On what did you base your
assumption? After all the previous discussions advising against it,
you are still just guessing.

This comes from a C reference I have. It is not kandr2 but it helps.

[...]

After your C reference other than K&R2 was not sufficiently helpful,
did you consider consulting K&R2?