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_FAILU RE);
}
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]

Re: code

On 2008-10-01, Bill Cunningham <nospam@nspam.i nvalidwrote:

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_FAILU RE);
}

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.

fprintf(fp, "%.2f\t%.2f\t%. 2f\t%.2f\n", a, b, c, d);
if ((fclose(fp)) == EOF)
perror(error);
return 0;
}
>

--
Andrew Poelstra apoelstra@wpsof tware.net
Only GOD may divide by zero. That is how he created black holes.
-Veselin Jungic

[Jack Klein]

Re: code

On Tue, 30 Sep 2008 21:36:14 -0400, "Bill Cunningham"
<nospam@nspam.i nvalidwrote in comp.lang.c:

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

Actually, Bill, it's not valid C89, as you said, the declarations are
misplaced.

#include <stdio.h>
#include <stdlib.h>
>
int main(int argc, char *argv[])
{
if (argc != 6) {
puts("print usage error");
exit(EXIT_FAILU RE);
}

Above is an executable statement.

FILE *fp;
char *error;
double a, b, c, d;

Here, after the first executable statement, you have a number of
variable definitions. C89 does not allow declarations or definitions
after any executable statements in a block. You are either using C99
or a compiler that allows this as an extension in its not strictly
conforming mode.

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]);

That has the advantage of showing the user what the unusable file name
was.

a = strtod(argv[1], NULL);
b = strtod(argv[2], NULL);
c = strtod(argv[3], NULL);
d = strtod(argv[4], NULL);

The code above is safe, but of course does not detect whether those
four values were actually valid representations of floating point
numbers.

fprintf(fp, "%.2f\t%.2f\t%. 2f\t%.2f\n", a, b, c, d);
if ((fclose(fp)) == EOF)
perror(error);

Again, fclose() is not required to set errno on failure, so there's no
telling what might be output on various implementations .

return 0;
}
>
Thanks all.
>
Bill

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

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

With the variable definitions after the first if block, the code is
conforming C99, but not C89. Other than that it is also correct C89.

There's no real point to the fopen() being under the strtod() calls as
the program is written, since the strtod() calls can't fail. All they
can do is to set a, b, c, and/or d to not particularly useful values
if the command line arguments do not represent proper numeric values.

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.

--
Jack Klein
Home: http://JK-Technology.Com
FAQs for
comp.lang.c http://c-faq.com/
comp.lang.c++ http://www.parashift.com/c++-faq-lite/
alt.comp.lang.l earn.c-c++

http://www.club.cc.cmu.edu/~ajo/docs/FAQ-acllc.html

[Keith Thompson]

Re: code

"Bill Cunningham" <nospam@nspam.i nvalidwrites:

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_FAILU RE);
}
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 (The_Other_Keit h) kst-u@mib.org <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"

[Bill Cunningham]

Re: code


"Keith Thompson" <kst-u@mib.orgwrote in message
news:lnprmlukbx .fsf@nuthaus.mi b.org...

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

Re: code


"Jack Klein" <jackklein@spam cop.netwrote in message
news:kim5e4ltj8 q6l7ragrgkq5bd3 t4cuf94sb@4ax.c om...

> 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_FAILU RE);
}

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

Bill

[William Pursell]

Re: code

On Oct 1, 3:20 am, Jack Klein <jackkl...@spam cop.netwrote:

On Tue, 30 Sep 2008 21:36:14 -0400, "Bill Cunningham" wrote:
>

    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]);
>
That has the advantage of showing the user what the unusable file name
was.

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 );
}

This still may produce a confusing error message on systems
on which fopen() doesn't set errno. Many examples have
been given on the group to deal with that. Stylistically,
I would recommend setting filename before the call to
fopen(), and discourage using argv[ 5 ] twice.

[Keith Thompson]

Re: code

"Bill Cunningham" <nospam@nspam.i nvalidwrites:

"Keith Thompson" <kst-u@mib.orgwrote in message
news:lnprmlukbx .fsf@nuthaus.mi b.org...

[...]

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

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.

--
Keith Thompson (The_Other_Keit h) kst-u@mib.org <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"

[Bill Cunningham]

Re: code


"William Pursell" <bill.pursell@g mail.comwrote in message
news:b47ec6a4-a730-472a-a9a5-d9e828cfb267@p5 9g2000hsd.googl egroups.com...

[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@gmail.com]

Re: code

On Oct 1, 10:18 pm, Keith Thompson <ks...@mib.orgw rote:

"Bill Cunningham" <nos...@nspam.i nvalidwrites:

<snip>

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

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]

Re: code

On Wed, 1 Oct 2008 13:58:26 -0400, "Bill Cunningham"
<nospam@nspam.i nvalidwrote:

>
>"Keith Thompson" <kst-u@mib.orgwrote in message
>news:lnprmlukb x.fsf@nuthaus.m ib.org...
>

>"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';

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.


--
Remove del for email

[Barry Schwarz]

Re: code

On Wed, 1 Oct 2008 14:30:50 -0400, "Bill Cunningham"
<nospam@nspam.i nvalidwrote:

>
>"Jack Klein" <jackklein@spam cop.netwrote in message
>news:kim5e4ltj 8q6l7ragrgkq5bd 3t4cuf94sb@4ax. com...
>

>> 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_FAIL URE);
>}
>
>Something like that is portable isn't it? That's my normal modus operandi.

Only with the appropriate includes and declarations.

--
Remove del for email

[Bill Cunningham]

Re: code


"Barry Schwarz" <schwarzb@dqel. comwrote in message
news:rnq7e4tte6 sb72cbpeeno7pje poj2n267p@4ax.c om...

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]

Re: code

On Wed, 1 Oct 2008 17:42:53 -0400, "Bill Cunningham"
<nospam@nspam.i nvalidwrote:

>
>"William Pursell" <bill.pursell@g mail.comwrote in message
>news:b47ec6a 4-a730-472a-a9a5-d9e828cfb267@p5 9g2000hsd.googl egroups.com...
>
>[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?

--
Remove del for email

[Barry Schwarz]

Re: code

On Wed, 1 Oct 2008 18:40:07 -0400, "Bill Cunningham"
<nospam@nspam.i nvalidwrote:

>
>"Barry Schwarz" <schwarzb@dqel. comwrote in message
>news:rnq7e4tte 6sb72cbpeeno7pj epoj2n267p@4ax. com...
>

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

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

--
Remove del for email