```c
while (1) {
fgets(expr, 200, stdin);
...
}
```
What happens if `fgets()` fail? You are ignoring its return value.
----------
```c
if (strncmp(expr, "quit", 4) == 0) {
break;
}
```
There's no way I'd have known to enter `quit` to exit had I not read the source code.
----------
```c
memset(expr, 0, strlen(expr));
```
What's the point of setting `expr` to all zero bytes when it is just going to be overwritten and null-terminated by `fgets()`?
----------
```
Node *parse_to_postfix(char *expr);
```
We wouldn't require this forward declaration if we define `parse_to_postfix()` before `main()`.
----------
```c
char *num_buf = calloc(strlen(expr) + 1, sizeof(char));
```
`sizeof(char)` is defined by the standard to be 1. You can safely elide it.
----------
```c
if (num_buf == NULL) {
perror("error:");
abort();
}
```
There's no need to cause *an abnormal termination* here. Simply do `exit(EXIT_FAILURE)`.
----------
```
if (isspace(c)) {
continue;
}
```
`isspace()` expects an `unsigned char`, not a `char`.
----------
```
Stack *output = stack_create(NULL);
Stack *op_stack = stack_create(NULL);
```
The return values go unchecked. Why return a null pointer on `malloc()` failure if you were simply going to ignore it?
----------
You are computing the length of `num_buf` thrice in `parse_to_postfix()`. Consider using an extra variable to save the length.
----------
```c
// debugging purposes
// Node *head = bottom;
// while (head != NULL) {
// node_print(head, 0);
// head = head->previous;
// }
// printf("\n");
```
This is no longer required.
----------
```c
printf("error: syntax error\n");
```
Error messages go to `stderr`, not `stdout`.
----------
```c
Node *a = stack_pop(stack), *b = stack_pop(stack);
long double result;
switch (expr->operator) {
case '+':
result = b->number + a->number;
break;
```
The above would invoke undefined behavior if `stack_pop()` returns `NULL`.
----------
```c
case '/':
result = b->number / a->number;
```
This would invoke undefined behavior if `a->number` was 0.
----------
```
Node *parse_to_postfix(char *expr);
```
`parse_to_postfix()` expects a non-null pointer, and it must be declared as such:
```
Node *parse_to_prefix(char expr[static 1]);
```
Same goes for the rest of the functions. This requires C99 albeit.
----------
```c
void node_print(Node *target, int newline) {
if (target == NULL) {
printf("cannot print NULL node\n");
return;
}
char nl = (newline) ? '\n' : ' ';
```
`newline` only needs to denote two states here, so it should be of type `bool`. (Include `stdbool.h` prior for the definition of `bool` prior to C2X.)
The parentheses around `newline` are also unneeded.
----------
```c
Node *new = malloc(sizeof(*new));
```
`new` is a bad identifier name since it is a reserved keyword in C++. Even if you never care for porting/compatibility with C++, you should still avoid naming identifiers after keywords from that language, since IDE style formatting tends to have a "C/C++" setting and then it will format `new` in weird ways.
Note that you can drop the parentheses around `*new`.
----------
```c
if (target == NULL) {
printf("error: cannot remove NULL node\n");
return NULL;
}
```
Instead of printing an error message to `stdout` - which should really have been `stderr` - just return `NULL`. Nobody wants a library to print random stuff to `stdout`/`stderr` in their code, especially without their knowledge.
----------
```c
int is_operator(char target) {
switch (target) {
case '+':
break;
case '-':
break;
case '/':
break;
case '*':
break;
case '^':
break;
default:
return 0;
}
return 1;
}
```
This over-engineered function can be simplified to:
```c
return strchr("+-/*^", target) != NULL;
```
Or you could use `memchr()`.
Or:
```c
return target == '+'
|| target == '-'
|| target == '*'
|| target == '/'
|| target == '^';
```
----------
```c
int get_precedence(char operator) {
switch (operator) {
case '+':
return 1;
case '-':
return 1;
case '/':
return 2;
case '*':
return 2;
case '^':
return 3;
default:
printf("error: cannot get precendence of unknown operator '%c'\n", operator);
return 0;
}
}
```
You could use a map for this:
```
int get_precedence(char operator) {
struct {
const char op;
const unsigned int precedence;
} ops = {
{'+', 1},
{'-', 1},
{'/', 2},
{'*', 2},
{...}
};
```
But that might be overkill. Or you could merge cases:
```c
case '+':
case '-':
return 1;
case '/':
case '*':
return 2;
case '*':
return 3;
default:
return 0;
```
----------
```c
int is_left_assoc(char operator) {
switch (operator) {
case '+':
return 1;
case '-':
return 1;
case '/':
return 1;
case '*':
return 1;
case '^':
return 0;
default:
printf("error: cannot get associativity of unknown operator '%c'\n", operator);
return 0;
}
}
```
Same goes for this:
```c
case '+': case '-': case '*': case '/':
return 1;
default:
return 0;
```
----------
```c
int node_check_valuetype(Node *target, enum NodeType type) {
return (target == NULL || target->value_type != type) ? 0 : 1;
}
```
This can be simplified to:
```c
return target && target->value_type == type;
```
### Some comments on the stack implementation:
I believe you have overly complicated it. Like @Davilsor suggests, a dynamic array with its top element at the end would have been more performant, more maintainable, and easier to implement. It would also have respected [principle of locality](https://en.wikipedia.org/wiki/Locality_of_reference).
I also do not like that `stack`'s internals are known to the client, i.e. its definition is included in `util.h`. I would have expected it to be an opaque type, with `get_top()`, `get_size()` et cetera.
The functions that are internal to the implementation should be defined as having internal linkage, i.e. marked `static` in the translation unit, and their declarations should be removed from the corresponding header file.
Consider using a common prefix for all the symbols. Currently, you have `stack_`, `node_`, et cetera.