Command line reverse polish calculator

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 
#include 
#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
#include "calc.h"

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

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

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

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

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

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

-
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

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

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

StackExchange Code Review Q#42537, answer score: 7