My K&R Ex 5-10 program, layout looks weird.

[G Patel]

Can people please comment on the layout/style of my problem? The major
issue I had was the layout. I ended up having to put a relatively
large switch statement, inside an if statement, which is inside a while
loop. If someone can tell me how I can rearrange these elements a
little to make it cleaner, I would appreciate that. I've tested it on
the command line, and it works *well*, but the source code layout is
bugging me.

I'd also like to add that I really appreciate all the help I've had for
my previous posts.

Thx all

/* expr K&R2 Ex 5-10 */

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

void push(double);
double pop(void);

int main(int argc, char *argv[])
{
double num;
double op2; /* for non-commutative binary operations */
char *p; /* for strtod */

if(argc == 1)
{
printf("Error, no arguments provided after program name\n");
exit(EXIT_FAILURE);
}

while((++argv)[0] != NULL)
{
num = strtod(argv[0], &p); /* will add errno checking later */
if(p != argv[0]) /* if valid number */
push(num);
else if(argv[0][1] != '\0') /* if more than one character */
{
printf("Error, %s not valid operator or operand\n",
argv[0]);
exit(EXIT_FAILURE);
}
else
switch(argv[0][0])
{
case '+':
push(pop() + pop());
break;
case '*':
push(pop() * pop());
break;
case '-':
op2 = pop();
push(pop() - op2);
break;
case '/':
op2 = pop();
if(op2 != 0.0)
push(pop() / op2);
else
printf("Error, division by zero\n");
exit(EXIT_FAILURE);
break;
case '%':
op2 = pop();
if(op2 != 0.0)
push((int)pop() % (int)op2);
else
printf("Error, division by zero\n");
exit(EXIT_FAILURE);
break;
default:
printf("Error, %c not valid operator or operand\n",
argv[0][0]);
exit(EXIT_FAILURE);
break;
}
}
printf("Top of Stack = %.8g\n", pop());
exit(EXIT_SUCCESS);
}

[Michael Mair]

G Patel wrote:

Can people please comment on the layout/style of my problem? The major
issue I had was the layout. I ended up having to put a relatively
large switch statement, inside an if statement, which is inside a while
loop. If someone can tell me how I can rearrange these elements a
little to make it cleaner, I would appreciate that. I've tested it on
the command line, and it works *well*, but the source code layout is
bugging me.

I'd also like to add that I really appreciate all the help I've had for
my previous posts.

Thx all

/* expr K&R2 Ex 5-10 */
Please give us an inkling what the exercise is about.
#include <stdio.h>
#include <stdlib.h>

void push(double);
double pop(void);
Where do push() and pop() come from -- your code does not compile
for me.

int main(int argc, char *argv[])
{
double num;
double op2; /* for non-commutative binary operations */
char *p; /* for strtod */

if(argc == 1)
Make this "argc <= 1" to also catch argc == 0.
{
printf("Error, no arguments provided after program name\n");
Error messages usually go to stderr.
exit(EXIT_FAILURE);
}

I miss some stack initialisation code.
while((++argv)[0] != NULL)
could also be "*++argv != NULL"
{
num = strtod(argv[0], &p); /* will add errno checking later */
if(p != argv[0]) /* if valid number */
push(num);
else if(argv[0][1] != '\0') /* if more than one character */
{
printf("Error, %s not valid operator or operand\n",
argv[0]);
exit(EXIT_FAILURE);
}
else
switch(argv[0][0])
{
What you are doing here is finding out the kind of token (number or
operator) and then acting on it.
So, depending on what you feel to be a better solution, you might
replace the above (apart from "push(num)") by
token = gettoken(*argv);
switch (token.type) {
with
struct Token {
enum { NUM, PLUS, MINUS, MUL, DIV, MOD, VALIDOPS, INVALID=-1 } type;
union {
double dblval;
} value;
} token;
case '+':
push(pop() + pop());
break;
case '*':
push(pop() * pop());
break;
case '-':
op2 = pop();
push(pop() - op2);
break;
case '/':
op2 = pop();
if(op2 != 0.0)
push(pop() / op2);
else
{
printf("Error, division by zero\n");
exit(EXIT_FAILURE);
}
Are you sure that this is actual code you tested???
break;
case '%':
op2 = pop();
if(op2 != 0.0)
push((int)pop() % (int)op2);
else
{
printf("Error, division by zero\n");
exit(EXIT_FAILURE);
}
dito
break;
default:
printf("Error, %c not valid operator or operand\n",
argv[0][0]);
exit(EXIT_FAILURE);
break;
}
This would not change much then (only that you would handle
case NUM, case PLUS ...)

Or you could do
if (token.type == INVALID) {
/* handle error */
}
else {
actontoken(token);
}
where actontoken() either incorporates the above switch or just calls
the respective function from an array of VALIDOPS function pointers
pointing to (in C99: inline-able) functions performing the work.
}
printf("Top of Stack = %.8g\n", pop());
exit(EXIT_SUCCESS);
}

The above suggestions are untested as you did not provide code to work
with.
Cheers
Michael
--
E-Mail: Mine is an /at/ gmx /dot/ de address.

[G Patel]

Michael Mair wrote:

G Patel wrote:

Can people please comment on the layout/style of my problem? The major issue I had was the layout. I ended up having to put a relatively
large switch statement, inside an if statement, which is inside a while loop. If someone can tell me how I can rearrange these elements a
little to make it cleaner, I would appreciate that. I've tested it on the command line, and it works *well*, but the source code layout is bugging me.

I'd also like to add that I really appreciate all the help I've had for my previous posts.

Thx all

/* expr K&R2 Ex 5-10 */
Please give us an inkling what the exercise is about.

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

void push(double);
double pop(void);

Where do push() and pop() come from -- your code does not compile
for me.

[Snipped}

Are you sure that this is actual code you tested???

No sorry, I left out the stack code.

I've added some of your suggestions and provided the "full" code for
the command-line based Reverse Polish Notation Calculator:

/* Command-line based RPN Calculator */
/* expr K&R2 Ex 5-10 */

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

void push(double);
double pop(void);

int main(int argc, char **argv)
{
double num;
double op2;
char *p; /* for strtod */

if(argc <= 1)
{
fprintf(stderr, "Error, no arguments provided\n");
exit(EXIT_FAILURE);
}

while(*++argv != NULL)
{
num = strtod(*argv, &p); /* lacks errno error checking */
if(p != *argv) /* if valid number */
push(num);
else if((*argv)[1] != '\0') /* if more than one character */
{
fprintf(stderr, "Error, %s not valid\n", *argv);
exit(EXIT_FAILURE);
}
else
switch(**argv) /* check first character only */
{
case '+':
push(pop() + pop());
break;
case '*':
push(pop() * pop());
break;
case '-':
op2 = pop();
push(pop() - op2);
break;
case '/':
op2 = pop();
if(op2 != 0.0)
push(pop() / op2);
else
fprintf(stderr, "Error, division by zero\n");
exit(EXIT_FAILURE);
break;
case '%':
op2 = pop();
if(op2 != 0.0)
push((int)pop() % (int)op2);
else
fprintf(stderr, "Error, division by zero\n");
exit(EXIT_FAILURE);
break;
default:
fprintf(stderr, "Error, %s not valid", **argv);
exit(EXIT_FAILURE);
break;
}
}
printf("Top of Stack = %.8g\n", pop());
exit(EXIT_SUCCESS);
}


#define MAXVAL 100 /* maximum depth of value stack */

int sp = 0; /* next free stack position */
double val[MAXVAL]; /* value stack */
/* push: push f onto value stack */
void push(double f)
{
if(sp < MAXVAL)
val[sp++] = f;
else
{
fprintf(stderr, "error: stack full, can't push %g\n", f);
exit(EXIT_FAILURE);
}
}

/* pop: pop and return top value from stack */
double pop(void)
{
if(sp > 0)
return val[--sp];
else
{
fprintf(stderr, "error: stack empty\n");
exit(EXIT_FAILURE);
}
}

[Michael Mair]

G Patel wrote:

Michael Mair wrote:

G Patel wrote:

Can people please comment on the layout/style of my problem? The
major
issue I had was the layout. I ended up having to put a relatively
large switch statement, inside an if statement, which is inside a
while
loop. If someone can tell me how I can rearrange these elements a
little to make it cleaner, I would appreciate that. I've tested it
on
the command line, and it works *well*, but the source code layout
is
bugging me.

I'd also like to add that I really appreciate all the help I've had
for
my previous posts.

Thx all

[snip]
Are you sure that this is actual code you tested???

No sorry, I left out the stack code.
I've added some of your suggestions and provided the "full" code for
the command-line based Reverse Polish Notation Calculator:

Here is the version I was talking about.
Note that I used C99 designated initialisers for clarity / avoiding
of errors. Just leave out the [....] = in the definition of TokenOp
and make ops = "\0+-*/%" for C89 code.
Typically, the token stuff would go into a translation unit of its
own (as would the stack code).

Now, main() looks pretty clear, doesn't it ;-)
Cheers
Michael
/* Command-line based RPN Calculator */
/* expr K&R2 Ex 5-10 */

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

#define DIE(str) \
do { \
fprintf(stderr, "Error: "str"\n"); \
exit(EXIT_FAILURE); \
} while (0)
#define DIE2(str, arg) \
do { \
fprintf(stderr, "Error: "str"\n", arg); \
exit(EXIT_FAILURE); \
} while (0)

void push(double);
double pop(void);

typedef struct {
enum { NUM, PLUS, MINUS, MUL, DIV, MOD, VALIDOPS, INVALID=-1 }
type;
union {
double dblval;
} value;
} InputToken;

InputToken GetToken (const char *string);

void OpStor (InputToken tok) { push(tok.value.dblval); }
void OpAdd (InputToken tok) { push(pop() + pop()); }
void OpSub (InputToken tok) { push(-pop() + pop()); }
void OpProd (InputToken tok) { push(pop() * pop()); }
void OpQuot (InputToken tok) {
double divisor = pop();
if (divisor) { push(pop() / divisor); }
else DIE("division by zero");
}
void OpRem (InputToken tok) {
long modulus = pop();
if (modulus) { push((long)pop() % modulus); }
else DIE("division by zero");
}

void (*TokenOp[VALIDOPS]) (InputToken) = {
[NUM] = OpStor, [PLUS] = OpAdd, [MINUS] = OpSub,
[MUL] = OpProd, [DIV] = OpQuot, [MOD] = OpRem
};
int main (int argc, char **argv)
{
if(argc <= 1) {
DIE("no arguments provided");
}

while (*++argv != NULL)
{
InputToken token = GetToken (*argv);

if (token.type == INVALID) {
DIE2("%s gives no valid token", *argv);
}
else {
TokenOp[token.type] (token);
}
}

printf("Top of Stack = %.8g\n", pop());
exit(EXIT_SUCCESS);
}

InputToken GetToken (const char *string)
{
double num;
char *p;
const char ops[VALIDOPS+1] = { [PLUS] = '+', [MINUS] = '-',
[MUL] = '*', [DIV] = '/', [MOD] = '%' };
InputToken tok;

num = strtod(string, &p); /* lacks errno error checking */
if(p != string) { /* if valid number */
tok.value.dblval = num;
tok.type = NUM;
}
else if ( (p=strchr(ops+PLUS, *string)) && string[1] == '\0' ) {
tok.type = p - ops;
}
else {
tok.type = INVALID;
}

return tok;
}
#define MAXVAL 100 /* maximum depth of value stack */

int sp = 0; /* next free stack position */
double val[MAXVAL]; /* value stack */
/* push: push f onto value stack */
void push(double f)
{
if(sp < MAXVAL)
val[sp++] = f;
else
{
fprintf(stderr, "error: stack full, can't push %g\n", f);
exit(EXIT_FAILURE);
}
}

/* pop: pop and return top value from stack */
double pop(void)
{
if(sp > 0)
return val[--sp];
else
{
fprintf(stderr, "error: stack empty\n");
exit(EXIT_FAILURE);
}
}
--
E-Mail: Mine is an /at/ gmx /dot/ de address.

[Luke Wu]

Michael Mair wrote:

[snipped request for assistance]]

Here is the version I was talking about.
Note that I used C99 designated initialisers for clarity / avoiding
of errors. Just leave out the [....] = in the definition of TokenOp
and make ops = "\0+-*/%" for C89 code.
Typically, the token stuff would go into a translation unit of its
own (as would the stack code).

Now, main() looks pretty clear, doesn't it ;-)

You're program looks horrible overall. Was it you purpose to purposely
make the code cryptic? What's the point of the union? What's the point
of all that fluff you have in it that isn't needed?

Your code fails miserably for serveral reasons (order of operation of
operands being unspecified, modulus on non integeral type being
undefined, etc, etc.). The code is ugly and it's littered with
mistakes and it is overly complicated for such a simple task.

Let this code be an example of HOW NOT TO CODE to anyone reading this
thread. How can you confidently post code like this, when the OP's
code is obviously more clear and doesn't fail on several conditions
like yours does?

Mike Mair's bad code:

/* Command-line based RPN Calculator */
/* expr K&R2 Ex 5-10 */

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

#define DIE(str) \
do { \
fprintf(stderr, "Error: "str"\n"); \
exit(EXIT_FAILURE); \
} while (0)
#define DIE2(str, arg) \
do { \
fprintf(stderr, "Error: "str"\n", arg); \
exit(EXIT_FAILURE); \
} while (0)
void push(double);
double pop(void);

typedef struct {
enum { NUM, PLUS, MINUS, MUL, DIV, MOD, VALIDOPS, INVALID=-1 }
type;
union {
double dblval;
} value;
} InputToken;

InputToken GetToken (const char *string);

void OpStor (InputToken tok) { push(tok.value.dblval); }
void OpAdd (InputToken tok) { push(pop() + pop()); }
void OpSub (InputToken tok) { push(-pop() + pop()); }
void OpProd (InputToken tok) { push(pop() * pop()); }
void OpQuot (InputToken tok) {
double divisor = pop();
if (divisor) { push(pop() / divisor); }
else DIE("division by zero");
}
void OpRem (InputToken tok) {
long modulus = pop();
if (modulus) { push((long)pop() % modulus); }
else DIE("division by zero");
}

void (*TokenOp[VALIDOPS]) (InputToken) = {
[NUM] = OpStor, [PLUS] = OpAdd, [MINUS] = OpSub,
[MUL] = OpProd, [DIV] = OpQuot, [MOD] = OpRem
};
int main (int argc, char **argv)
{
if(argc <= 1) {
DIE("no arguments provided");
}

while (*++argv != NULL)
{
InputToken token = GetToken (*argv);

if (token.type == INVALID) {
DIE2("%s gives no valid token", *argv);
}
else {
TokenOp[token.type] (token);
}
}

printf("Top of Stack = %.8g\n", pop());
exit(EXIT_SUCCESS);
}

InputToken GetToken (const char *string)
{
double num;
char *p;
const char ops[VALIDOPS+1] = { [PLUS] = '+', [MINUS] = '-',
[MUL] = '*', [DIV] = '/', [MOD] = '%' }; InputToken tok;

num = strtod(string, &p); /* lacks errno error checking */
if(p != string) { /* if valid number */
tok.value.dblval = num;
tok.type = NUM;
}
else if ( (p=strchr(ops+PLUS, *string)) && string[1] == '\0' ) {
tok.type = p - ops;
}
else {
tok.type = INVALID;
}

return tok;
}
#define MAXVAL 100 /* maximum depth of value stack */

int sp = 0; /* next free stack position */
double val[MAXVAL]; /* value stack */
/* push: push f onto value stack */
void push(double f)
{
if(sp < MAXVAL)
val[sp++] = f;
else
{
fprintf(stderr, "error: stack full, can't push %g\n", f);
exit(EXIT_FAILURE);
}
}

/* pop: pop and return top value from stack */
double pop(void)
{
if(sp > 0)
return val[--sp];
else
{
fprintf(stderr, "error: stack empty\n");
exit(EXIT_FAILURE);
}
}

[Michael Mair]

Luke Wu wrote:

Michael Mair wrote:

[snipped request for assistance]]

Here is the version I was talking about.
Note that I used C99 designated initialisers for clarity / avoiding
of errors. Just leave out the [....] = in the definition of TokenOp
and make ops = "\0+-*/%" for C89 code.
Typically, the token stuff would go into a translation unit of its
own (as would the stack code).

Now, main() looks pretty clear, doesn't it ;-)
You're program looks horrible overall. Was it you purpose to purposely
make the code cryptic? What's the point of the union? What's the point
of all that fluff you have in it that isn't needed?

I never claimed the code was good in that respect. IMO, the OP's
first shot at it was good enough; however, I wanted to present
an alternative which was different enough to see everything "in
between".
As for the additional "fluff": unions store logically equivalent
objects. The only "value" we have as of now, is the value of a
NUM token; making value a union in the first place provides a
way for extension. Much of the "fluff" is not needed but demonstrates
how you can move on.

Your code fails miserably for serveral reasons (order of operation of
operands being unspecified, modulus on non integeral type being
undefined, etc, etc.).
You did not look at my code. _In_contrast_ to the OP, I use a
"long modulus" which gives the correct division by zero check
whereas the OP's code will not catch "2 .5 %".
Otherwise, the order of "operation of operands" (you probably
mean evaluation) is specified and the treatment is equivalent
to the OP's code.
The code is ugly and it's littered with
mistakes and it is overly complicated for such a simple task.
Please point out the mistakes I made in addition to those present
in the OP's code.
Yes, it is overly complicated. However, it seems this threw you
off the mark.

Let this code be an example of HOW NOT TO CODE to anyone reading this
thread.
This is up to anyone.
How can you confidently post code like this, when the OP's
code is obviously more clear and doesn't fail on several conditions
like yours does?
Please point out the several conditions the OP's code handles without
failure which mine does not handle.

Mike Mair's bad code:

Please do not mangle my name.
[snip: my bad code]
-Michael
--
E-Mail: Mine is an /at/ gmx /dot/ de address.

[Michael Mair]

Michael Mair wrote:

Luke Wu wrote:

Michael Mair wrote:

[snipped request for assistance]]

Here is the version I was talking about.
Note that I used C99 designated initialisers for clarity / avoiding
of errors. Just leave out the [....] = in the definition of TokenOp
and make ops = "\0+-*/%" for C89 code.
Typically, the token stuff would go into a translation unit of its
own (as would the stack code).

Now, main() looks pretty clear, doesn't it ;-)

You're program looks horrible overall. Was it you purpose to purposely
make the code cryptic? What's the point of the union? What's the point
of all that fluff you have in it that isn't needed?

I never claimed the code was good in that respect. IMO, the OP's
first shot at it was good enough; however, I wanted to present
an alternative which was different enough to see everything "in
between".
As for the additional "fluff": unions store logically equivalent
objects. The only "value" we have as of now, is the value of a
NUM token; making value a union in the first place provides a
way for extension. Much of the "fluff" is not needed but demonstrates
how you can move on.

Your code fails miserably for serveral reasons (order of operation of
operands being unspecified, modulus on non integeral type being
undefined, etc, etc.).

You did not look at my code. _In_contrast_ to the OP, I use a
"long modulus" which gives the correct division by zero check
whereas the OP's code will not catch "2 .5 %".
Otherwise, the order of "operation of operands" (you probably
mean evaluation) is specified and the treatment is equivalent
to the OP's code.

Sorry, the - op of course is fouled up in my code.

The code is ugly and it's littered with
mistakes and it is overly complicated for such a simple task.

Please point out the mistakes I made in addition to those present
in the OP's code.
Yes, it is overly complicated. However, it seems this threw you
off the mark.

Let this code be an example of HOW NOT TO CODE to anyone reading this
thread.

This is up to anyone.

How can you confidently post code like this, when the OP's
code is obviously more clear and doesn't fail on several conditions
like yours does?

Please point out the several conditions the OP's code handles without
failure which mine does not handle.

Mike Mair's bad code:

Please do not mangle my name.
[snip: my bad code]
-Michael

--
E-Mail: Mine is an /at/ gmx /dot/ de address.

[Luke Wu]

Michael Mair wrote:

Luke Wu wrote:

Michael Mair wrote:

Please point out the mistakes I made in addition to those present
in the OP's code.

Order of operationg of pop() in -, /, %. Not sure which bugs were
present in the OP's post, but if you did solve these bugs then I
commend you for doing something good at least.

Yes, it is overly complicated. However, it seems this threw you
off the mark.

Let this code be an example of HOW NOT TO CODE to anyone reading

this thread.

This is up to anyone.

How can you confidently post code like this, when the OP's
code is obviously more clear and doesn't fail on several conditions
like yours does?

Please point out the several conditions the OP's code handles without
failure which mine does not handle.

Compile your code on serveral systems/compilers and test these:

1 2 -
6 3 /
5 3 %
Your program doesn't even work for the simplest cases after I compiled
it. Just because it works for you doesn't mean it will on others'
(your compiler must be evaluating the pop()s left to right). The fact
that you don't realize that order of evaluation of operands (pop() in
this case) is unspecified means you aren't good enough to show
alternatives to code that seems "okay" in the first place.

[Michael Mair]

Luke Wu wrote:

Michael Mair wrote:

Luke Wu wrote:

Michael Mair wrote:

Please point out the mistakes I made in addition to those present
in the OP's code.

Order of operationg of pop() in -, /, %. Not sure which bugs were
present in the OP's post, but if you did solve these bugs then I
commend you for doing something good at least.

:-)

- I found myself.

However, /, % I do not see.

[snip] Compile your code on serveral systems/compilers and test these:

1 2 -
Granted. I happily forgot about the sequence point (better, the
lack thereof).
6 3 /
I expect output 6/3 -> 2.
My stack contains
6, 3, then I call

void OpQuot (InputToken tok) {
double divisor = pop();
if (divisor) { push(pop() / divisor); }
else DIE("division by zero");
}

divisor <- 3, stack: 6,
then I divide 6/divisor -> 2 and push(2)
Where does this go wrong?

Under linux, this compiles and runs happily with gcc and icc,
under Windows, this works with cygwin/gcc (I do not have
other compilers available right now).
The behaviour is exactly the same as for the OP's code.
5 3 %
void OpRem (InputToken tok) {
long modulus = pop();
if (modulus) { push((long)pop() % modulus); }
else DIE("division by zero");
}

dito. Also, I do not see the problem with the modulus and the
operation as such you were talking about.

Your program doesn't even work for the simplest cases after I compiled
it. Just because it works for you doesn't mean it will on others'
(your compiler must be evaluating the pop()s left to right). The fact
that you don't realize that order of evaluation of operands (pop() in
this case) is unspecified means you aren't good enough to show
alternatives to code that seems "okay" in the first place.

As I posted _before_ your reply, I found the error with the "-"
(not looking at my code, just thinking about it).
I do not see any structural error for / and %, though.
For + and *, the order of evaluation does not play a role.

Please show me where / and % go wrong. There is a sequence point in
between the pop() calls so I do not see any problem there.
-Michael
--
E-Mail: Mine is an /at/ gmx /dot/ de address.