Command line reverse polish calculator

Question

Write the program expr, which evaluates a reverse Polish expression from the command line, where each operator or operand is a separate argument.

For example:

expr 2 3 4 + *

There are 4 files:

calc.c

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

int main(int argc, char *argv[]) {
    double tmpOp;
    char c;

    if(argc > 3) {

        while(--argc) {
            c = getOp(*++argv);
            switch(c) {
                case NUMBER:
                    push(atof(*argv));
                    break;
                case '+':
                    push(pop() + pop());
                    break;
                case '-':
                    tmpOp = pop();
                    push(pop() - tmpOp);
                    break;
                case '*': 
                    push(pop() * pop());
                    break;
                case '\\':
                    tmpOp = pop();

                    if(!tmpOp) {
                        printf("error: can't divide by 0\n");
                    }
                    else {
                        push(pop()/tmpOp);
                    }
                    break;
                default:
                    printf("error: invalid expression %s\n", *argv);
            }
        }
        printf("%g\n", pop());
    }
    else {
        printf("invalid call\n");
    }

    return 0;
}

This file consist in a while loop that runs argc-1 times. At each iteration an argument is analized. If it is a number, it is pushed to the stack. If it is an operator, 2 numbers are popped from the stack and the operator is applied to these operands. If the argument is not valid, an error is printed.

argc should allways be bigger than 3 because, at this moment, the calculator needs at least 2 operands and 1 operator for an valid call.

The file getop.c:

#include <ctype.h>
#include "calc.h"

static int isNumber(const char *s) {
    int i = 0;

    if(isdigit(s[i])) {
        for(; isdigit(s[i]); i++) 
            ;
    }

    if(s[i] == '.') {
        i++;
        for(; isdigit(s[i]); i++)
            ;
    }

    if(s[i] == 'e' || s[i] == 'E') {
        i++;
        for(; isdigit(s[i]); i++)
            ;
    }

    if(s[i] != '\0') {
        return 0;
    }
    return 1;
}


char getOp(const char *op) {
    int i;

    if(!isdigit(op[0]) && op[0] != '.') {
        return op[0];
    }

    i = 0;

    /* support for negative number */
    if(op[0] == '-' && !isdigit(op[1])) {
        return op[0];
    }
    else {
        i++;
    }
    if(isNumber(op + i)) {
        return NUMBER;
    }

    return op[0];
}

The function getOp returns 'n' if op contains an valid number or the first charachter of op if it is not a number - for example when we are dealing with operators. The function getOp uses a helper to check if the the value of op is a valid number. Also, the call getOp(op + i) is intended to validate checks for negative number.

The file stack.c:

#include <stdio.h>
#define STACKVALUE 100


static double stack[STACKVALUE];
static double *pStack = stack; /* next free position in stack */

void push(double value) {
    if(pStack - stack > STACKVALUE) {
        printf("error: there is not enough psace in stack");
    }
    else {
       *(pStack++) = value; 
    }
}

double pop(void) {
    if((pStack - stack) > 0) {
        return *--pStack;
    }

    printf("warning: the stack is free");
    return 0.0;
}

And the file calc.h:

#define NUMBER 'n'

int getOp(const char *s);
void push(double value);
double pop(void);

Answer 1

What you did well

Taking advantage of the shell to split your expression into tokens is smart. It saves you from the trouble of having to write a tokenizer.

Beware, though, of a usability issue with Unix shells, where * and \ have special significance and need to be quoted or escaped.

Bugs

1. Compilation error: This was just carelessness.

   getop.c:31:6: error: conflicting types for 'getOp'
   char getOp(const char *op) {
        ^
   ./calc.h:3:5: note: previous declaration is here
   int getOp(const char *s);
       ^
   1 error generated.
   

2. Error handling: You treat errors like warnings. If there's an error, I would expect the program to print nothing to stdout, and for the program to exit with a non-zero status code. Error and warning messages should be printed to stderr instead of stdout.

3. Picky validation: In my opinion, this should successfully evaluate to 1:

   $ ./expr 1
   invalid call
   

   By the way, if you find an insufficient number of command-line arguments, just return early.

   int main(int argc, char *argv[]) {
       if (argc <= 1) {
           fprintf(stderr, "invalid call\n");
           return 1;
       }
   
       while (--argc) {
           …
       }
       printf("%g\n", pop());
       return 0;
   }
   

4. Mishandling of negative numbers: I would expect the following calculation to yield -1:

   $ ./expr 3 -4 +
   warning: the stack is freewarning: the stack is free-3
   

5. Unconventional division operator: It is customary to use / for division. I don't know why you use \ instead.

6. Division-by-zero paranoia: In contrast to integer arithmetic, where division by zero is undefined behaviour, division by zero in IEEE 754 floating point arithmetic should just return infinity. Perhaps you could remove the check for division by zero.

Number parsing

I suggest the following implementation for getop.c, which is simpler and handles negative numbers.

#include <ctype.h>
#include "calc.h"

static int isNumber(const char *s) {
    if (*s == '-' || *s == '+') s++;
    if (*s == '\0') {
        return 0;
    }

    while (isdigit(*s)) s++;

    if (*s == '.') {
        s++;
        while (isdigit(*s)) s++;
    }

    if (*s == 'e' || *s == 'E') {
        s++;
        while (isdigit(*s)) s++;
    }

    return *s == '\0';
}


char getOp(const char *op) {
    return isNumber(op) ? NUMBER : *op;
}