This review is unfinished, but already quite long. I'll probably write the rest in the next few hours.
Well, this is pretty decent code, and a good attempt at making a simple RPN calculator. Let me take a more thorough look at it, with my compiler options that should be enabled by default:
-Wall -Wextra -Wfloat-equal -Winline -Wundef -Werror -fverbose-asm -Wint-to-pointer-cast \ -Wshadow -Wpointer-arith -Wcast-align -Wcast-qual -Wunreachable-code -Wstrict-overflow=5 \ -Wwrite-strings -Wconversion --pedantic-errors -ggdb -Wredundant-decls \ -Wstrict-prototypes -Wmissing-prototypes
With -Werror=maybe-uninitialized for GCC and -Werror=uninitialized for Clang.
This code, however, does not compile with a sane configuration.
main.c:30:1: error: return type defaults to ‘int’ [-Wimplicit-int] main () { ^~~~ main.c:30:1: error: function declaration isn’t a prototype [-Werror=strict-prototypes] main.c: In function ‘main’: main.c:56:25: error: comparing floating point with == or != is unsafe [-Werror=float-equal] if (_op != 0.0) ^~ main.c:63:25: error: comparing floating point with == or != is unsafe [-Werror=float-equal] if (_op != 0.0) ^~ main.c: In function ‘getop’: main.c:120:20: error: conversion to ‘char’ from ‘int’ may alter its value [-Werror=conversion] while ((s[0] = c = getch()) == ' ' || c == '\t') ^ In file included from main.c:3:0: main.c:129:33: error: conversion to ‘char’ from ‘int’ may alter its value [-Werror=conversion] while (isdigit(s[++i] = c = getch())) ^ main.c:132:33: error: conversion to ‘char’ from ‘int’ may alter its value [-Werror=conversion] while (isdigit(s[++i] = c = getch())) ^ main.c: In function ‘ungetch’: main.c:151:23: error: conversion to ‘char’ from ‘int’ may alter its value [-Werror=conversion] buf[bufp++] = c; ^ cc1: all warnings being treated as errors makefile:2: recipe for target 'main' failed make: *** [main] Error 1
We'll go over the code and look at what's causing this to not compile.
#define NUMBER '0'
A comment explaining this name would be helpful, or just a better name.
int sp = 0; double stack[MAXSTACK]; char buf[BUFSIZE]; int bufp = 0; double x = 0;
Avoid global variables, as implicit global state should be avoided whenever possible, especially in small programs that have no need of it anyways.
Additionally, the descriptiveness of a variable name should scale with its scope. i, j, k, etc are good for loop counters (especially since programmers know i and j will be loop counters); for function scope, depending on the inteded lifetime of the variable, you should use at least a word or two, perhaps shortened.
For global variables, especially those globals that have little cause to exist, you should justify their existence with better names. buf could at least be input_buf, but that's still a little vague for a global. I'd go with input_buf_ptr (bufp) and stack_ptr (sp).
Finally, x is a god awful variable name for any scope or lifetime longer than a couple lines.
main.c:30:1: error: return type defaults to ‘int’ [-Wimplicit-int] main () { ^~~~ main.c:30:1: error: function declaration isn’t a prototype [-Werror=strict-prototypes]
Two errors for one simple declaration, because...
main () { ... }
This does not declare a function, except in C89.
int main () { ... }
Neither does this. function declaration isn’t a prototype [-Werror=strict-prototypes]
int main (void) { ... }
This does declare and define a function in C99 and C11, which I hope you are using. See Is it better to use C void arguments “void foo(void)” or not “void foo()”?
main must be declared int except if its return type is omitted, in which case it's inferred to be int by the compiler. Compliant programs declare int main.
Since C99, main need not explicitly return int -- if you omit a return value, the compiler will insert return EXIT_SUCCESS; at the end.
int type; double _op; char s[MAXOP];
These are poor variable names -- you are not limited to 6 characters, like assemblers of old! Seriously, there's no excuse for these to be named this way.
I can only guess that type is the determined type of the input, op is the determined resultant operator, and s is the raw input string, but I shouldn't have to guess. Use better names, and then use comments for more complex ideas.
while ((type = getop(s)) != EOF) {
C is very cool in that you can do this in one line, but that doesn't mean you should do this if you don't have to -- and you don't have to. Here's one possibility:
char s[MAXOP]; int type; double op; do { type = getop(s); // ... } while (EOF != type);
It's still a little messy which means the whole flow deserves refactoring, in my opinion.
switch (type) { case NUMBER:
Ahh, maybe it should be called OP_NUMBER or so? And why does '0' get a name while '+' and the other operators don't? No apparent reason, so far.
case '/': _op = pop(); if (_op != 0.0) push(pop() / _op); else printf(CRED "ERROR: Cannot divide by 0\n" CRESET); break;
Fact: you cannot reliably compare floating point values with == or !=. Instead, you need to use an epsilon and compare the difference, like:
abs(a - b) < 1e-8
This is what I'm used to using, but apparently the following is better:
abs(( a - b ) / b) < 1e-8
I've never actually seen this before, but apparently it's not good enough for the real pedants either. You can, however, use the first, simpler one, and satisfy most input cases as well as the compiler, who is right to warn.
See What Every Programmer Should Know About Floating Point Math, and The Floating Point Guide.
Opinion: omitting braces around control statements like if is dangerous and ugly. The example oft provided is to suppose that someone, be it you or not, comes along and changes the else branch:
else printf(CRED "ERROR: Cannot divide by 0\n" CRESET); abort();
Well, now the program aborts 100% of the time, and it will take you a couple minutes to figure out why because of the tricky indentation.
If you had used braces, this would have been avoided:
else { printf(CRED "ERROR: Cannot divide by 0\n" CRESET); abort(); }
Futhermore, and perhaps even more extreme, I always use block-style case:
case j: { printf("%s\n", x); break; }
It looks better to me, and feels much less haphazard than braceless case.
case 'x': x = pop(); break; case 'X':
Whaaat? Why the letter x? Why does letter case matter? Comments would be helpful.
void push(double f) { ... }
f is not returned or modified, nor should it be. As such, it should be declared const, so that the compiler may make optimizations and assumptions around the fact it will not change:
void push (const double f) { ... }
if (sp < MAXSTACK) stack[sp++] = f; else printf(CRED "ERROR: Stack is full, cannot push %g\n" CRESET, f);
In these four lines, six names are used. A total of eight references are made to names from this translation unit, but only one of these names was declared anywhere near this code.
I know what f is; it's a mutable double. For the other names, I have to think a little bit, to remember where they came from -- this is bad, because it should be explicit and obvious where they came from.
I still can't think why those need to be globals, but nevermind that.
Ditto on the braces, too.
double top(void) {
Alright, now there's a declaration!
if (sp > 0) return stack[sp - 1]; else { return 0.0; }
I semantically disagree with making an empty stack return 0.0 forever, but that is just personal style.
double pop(void) { if (sp > 0) return stack[--sp]; else { printf(CRED "ERROR: Stack is empty\n" CRESET); return 0.0; } }
Rather oddly and inconsistently, getting the top of an empty stack is fine and gives 0.0 but popping from an empty stack is an error, and gives 0.0.
