"The python that ate a calculator"

In a YouTube video I watched, the speaker said it is important to know how to calculate between numbers of different bases; i.e. two, ten, and sixteen. I was then inspired to write a small C program to ensure I understand how such conversions are done. I then wrote a short Python2 script to test the program. I'd like any constructive feedback on the coding style, layout, efficiency, and anything else that stands out. Debug lines are commented out.

calc.c

#include 
#include 
#include 

typedef enum {
    AND = '&',
    OR = '|',
    XOR = '^',
    NONE = 0
} operator;

typedef enum {
    B02 = 'b',
    B10 = 'd',
    B16 = 'x'
} type;

const char zero = '0';
const char _A = 'A';
const char _a = 'a';

int main(int argc, char **argv) {
    int i, offs, len;
    char *nend;
    long result = 0, tmp, optmp;
    operator op = NONE;
    type ntype;

    for (i = 1; i = _a) tmp -= _a - 10;
                else if (tmp >= _A) tmp -= _A - 10;
                else tmp -= zero;
                tmp *= pow(16, offs);
                break;
            }
            optmp += tmp;
            offs++;

        }
//printf("%s: offs: %i tmp: %li optmp: %li len: %i result: %li\n", argv[i], offs, tmp, optmp, len, result);
        switch (op) {
        case AND:
            result &= optmp;
            break;
        case OR:
            result |= optmp;
            break;
        case XOR:
            result ^= optmp;
            break;
        case NONE:
            result += optmp;
            break;
        }
//printf("\n");
    }
    printf("%li\n", result);
    return 0;
}

makecalc.sh

gcc -Wall calc.c -o calc

testcalc.py

```
import subprocess as subp
import time

_bin = lambda x: bin(x)[2:] + 'b'
_dec = lambda x: str(x) + 'd'
_hex = lambda x: hex(x)[2:] + 'x'

ops = ('&', '|', '^')
types = (_bin, _dec, _hex)
RANGE = 20

tmp = None

def test(op, t, x, y):
call = ["./calc", t(x), op, t(y)]
print call
proc = subp.Popen(call, stdout=subp.PIPE, stdin=su

I suppose you don't indent the printf debug lines to make them easier to spot. That's a sensible idea (although not one I've seen before). However, I treat Code Review answers as a "just before checkin" thing, so I'd suggest removing these by this point.

You pre-declare your variables. Don't do this; write them as close to point-of-use as reasonable.

Your NONE op seems to be rather useless - you could just use OR instead. So remove it.

Your

const char zero = '0';
const char _A = 'A';
const char _a = 'a';

variables might seem like a good adherence to "no magic constants", but in reality they have effectively no useful purpose. If you wanted to update your code to support other character sets, you'd want to redesign the whole parser anyway. IMHO, it's simpler to inline these.

You're not required to return 0; in modern C, and it's somewhat typical not to.

Your integer parser should be generic in the base. I suggest extracting this into a function like int parse_digit(char character, int base, int place):

int parse_digit(char character, int base, int place) {
    int value;

    if (character >= 'a') {
        value = character - 'a' + 10;
    }
    else if (character >= 'A') {
        value = character - 'A' + 10;
    }
    else {
        value = character - '0';
    }

    return value * pow(base, place);
}

while (offs < len - 1) {
    optmp += parse_digit(*(nend - offs), base, offs);
    offs++;
}

base can be calculated with a simple lookup on ntype:

int type_to_base(type ntype) {
    switch (ntype) {
    case B02: return 2;
    case B10: return 10;
    case B16: return 16;
    }
}

Although this makes the type enum rightly look silly: it's just an integer after all!

int flag_to_base(char flag) {
    switch (flag) {
    case 'b': return 2;
    case 'd': return 10;
    case 'x': return 16;
    }
}

Note that this is a somewhat silly way to do things, though. First of all, pow takes doubles! I'd rather avoid that if possible. Further, this is more work than needs to be done. Instead, one can do this:

1*base^4 + 0*base^3 + 1*base^2 + 0*base^1 + 1*base^0  // how you're currently doing it
= (((1 * base + 0) * base + 1) * base + 0) * base + 1          // simpler way to do it

This can be done as:

int parse_digit(char character) {
    if (character >= 'a') {
        return character - 'a' + 10;
    }
    else if (character >= 'A') {
        return character - 'A' + 10;
    }
    else {
        return character - '0';
    }
}

while (offs < len - 1) {
    optmp *= base;
    optmp += parse_digit(*(nend - offs));
    offs++;
}

Parsing integers should be in another function, and can be slightly simplified:

long parse_int(char *start, char *end) {
    int base = flag_to_base(*end);

    long ret = 0;
    for (char *digit = start; digit < end; digit++) {
        ret *= base;
        ret += parse_digit(*digit);
    }

    return ret;
}

I would also put application of optmp into a function, and stop calling it optmp (in general, most variables are temporary - stating this is pointless):

long apply(operator op, long left, long right) {
    switch (op) {
    case AND: return left & right;
    case OR:  return left | right;
    case XOR: return left ^ right;
    }
}

I would then change (generally) long to uint64_t and int to uint32_t (or even uint8_t) - there's no reason to be using variable-length integers in this day and age for these types, and you don't need signed values. When you do want variable-length values, you'd probably rather stick with size_t. (If you find these unwieldy, typedef them to u64, u32 and u8. Just don't use int and long 'cause they're prettier.)

Compiling with warnings, I get

calc.c: In function ‘apply’:
calc.c:18:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
calc.c: In function ‘flag_to_base’:
calc.c:26:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^

This is because the new method allows better case analysis - we've forgotten to check that such operations are actually valid!

```
#include
#include
#include
#include

typedef enum {
AND = '&',
OR = '|',
XOR = '^',
} operator;

int apply(operator op, uint64_t *left, uint64_t right) {
switch (op) {
case AND: *left &= right; break;
case OR: *left |= right; break;
case XOR: *left ^= right; break;
default: return -1;
}
return 0;
}

int flag_to_base(char flag, uint8_t *base) {
switch (flag) {
case 'b': *base = 2; break;
case 'd': *base = 10; break;
case 'x': *base = 16; break;
default: return -1;
}
return 0;
}

int parse_digit(char digit, uint8_t *value) {
if ('a' = base) {
return -1;
}

ret = base;
*ret += value;
}

return 0;
}

int main(int argc, char **argv) {
uint64_t result = 0;
operator op = OR;

for (int i = 1; i < argc;

StackExchange Code Review Q#93576, answer score: 21