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

Discussion in 'C Programming' started by G Patel, Mar 10, 2005.

  1. G Patel

    G Patel Guest

    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);
    }
    G Patel, Mar 10, 2005
    #1
    1. Advertising

  2. G Patel

    Michael Mair Guest

    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.
    Michael Mair, Mar 10, 2005
    #2
    1. Advertising

  3. G Patel

    G Patel Guest

    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);
    }
    }
    G Patel, Mar 10, 2005
    #3
  4. G Patel

    Michael Mair Guest

    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.
    Michael Mair, Mar 10, 2005
    #4
  5. G Patel

    Luke Wu Guest

    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);
    > }
    > }
    >
    Luke Wu, Mar 11, 2005
    #5
  6. G Patel

    Michael Mair Guest

    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, Mar 11, 2005
    #6
  7. G Patel

    Michael Mair Guest

    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.
    Michael Mair, Mar 11, 2005
    #7
  8. G Patel

    Luke Wu Guest

    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.
    Luke Wu, Mar 11, 2005
    #8
  9. G Patel

    Michael Mair Guest

    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.
    Michael Mair, Mar 11, 2005
    #9
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Rick Spiewak
    Replies:
    3
    Views:
    3,131
    Rick Spiewak
    Aug 26, 2003
  2. Geoff Hague
    Replies:
    1
    Views:
    320
  3. Replies:
    1
    Views:
    562
    John Timney \(MVP\)
    Jun 19, 2006
  4. Skybuck Flying

    nrand48 source code looks weird ?

    Skybuck Flying, Jul 7, 2004, in forum: C Programming
    Replies:
    10
    Views:
    647
    Kenneth Brody
    Jul 9, 2004
  5. Smith
    Replies:
    1
    Views:
    303
    Juan T. Llibre
    Nov 24, 2008
Loading...

Share This Page