HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

Number converter between bases

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
betweennumberconverterbases

Problem

I just wrote a utility program in C that converts numbers of different bases to 'standard' well-known bases (hex, dec, oct, bin). I called it repnum, short for "represent number."

You will notice a 'template' that I like to employ when writing C programs. Any suggestions?

repnum.h

``
#ifndef REPNUM_H
#define REPNUM_H

#ifdef __GNUC__
#define _GNU_SOURCE
#elif ! (_POSIX_C_SOURCE >= 2 || _XOPEN_SOURCE)
#define NO_OPTS
#endif

#define BUF_SIZE (1024)
#define PROG_VERSION (1.0f)
#define PROG_NAME "repnum"

#define iseven(n) (n % 2 == 0)
#define isodd(n) (n % 2 != 0)

#define _reporterror(c, ...) \
err(c, __VA_ARGS__)
#define _throwerror(c, ...) \
errx(c, __VA_ARGS__)

/* Not needed for now. Just for later use.
#ifdef NDEBUG
#define _assert(c, ...) \
c ? 0 : _throwerror(2, __VA_ARGS__)
#else
#include
#define _assert(c, ...) \
assert(c)
#endif
*/

/*
* Converts a number to a string containing its binary representation.
*
num is the number to convert.
*
buf is the string buffer to use.
*
n is the length of the string.
* Returns a pointer to the start of the representation. NOT THE START OF
buf.
* If NULL was returned, then
n characters are not enough and the function stopped at the middle of processing.
* NOTE: the algorithm used writes binary right to left, so to ensure highest performance,
* characters are written starting by the end of the string.
*/
char num2bin(unsigned long long, char , size_t);

/*
* Converts a string to a long.
* Converts
str as a base base number to long and stores it in num.
* Supports all bases that strtoull supports.
* Returns:
* 0 - Whole
str is valid. num is changed.
* 1 - Whole
str is invalid. num is not changed.
* 2 -
num is null or str is null or the first character in str is a null byte. num is not changed.
* 3 - Partial
str is valid. num is changed.
* 4 - Overflow.
num` is not changed.
*/
int str2ull(char , unsigned long long , int);

/*

Solution

Buffer overflow

char tmp[21];

if (b)
    sprintf(tmp, "Base %d is required.%c", b, 0);


If b is 10, then you will print 22 characters here. I'm not sure why you used %c to terminate the string, but it adds an extra null character to your string (the string becomes terminated with double null characters). If you had just done this, it would have been fine:

sprintf(tmp, "Base %d is required.", b);


But rather than just make your string perfectly the right size, I suggest making it a lot larger. You never know how things might change in the future (for example, you start supporting bases past 100). Memory is cheap, and there's no difference between a 21 character buffer and a 32 (or 64) character buffer.

Magic numbers

The statuses returned by str2ull are 1, 2, 3, and 4. These are meaningless, and a year from now you won't remember what they mean either. You should use identifiers instead of numbers. For example:

enum status {
    STATUS_OK,
    STATUS_BAD_ARGUMENT,
    STATUS_EMPTY_ARGUMENT,
    STATUS_EXTRA_CHARS,
    STATUS_RANGE_ERROR,
};

switch (status)
{
    case STATUS_BAD_ARGUMENT:
    case STATUS_EXTRA_CHARS:
        _throwerror(1, "%s is not a valid number.%s", s, b ? tmp : "");
    case STATUS_RANGE_ERROR:
        _throwerror(1, "%s is a too large number.", s);
}


Precision truncated

There is a problem in str2ull() when you get the value from strtoull():

int status;

if(!(status = strtoull(str, &end, base)) && (end == str))
    return 1;


Notice that you store the unsigned long long return value of strtoull() into status, which is an int. I'm not actually sure why you need that status variable in the first place, since you never set it to an actual status. I removed status and changed that line to this:

if(!(*num = strtoull(str, &end, base)) && (end == str))
    return 1;


Strange behavior with no arguments

I ran your program with no arguments. Rather than print the help info, it gave me some random output:


[dec] 1629102960 = [hex] 611a2370 [oct]

14106421560 [bin] 1100001000110100010001101110000

Code Snippets

char tmp[21];

if (b)
    sprintf(tmp, "Base %d is required.%c", b, 0);
sprintf(tmp, "Base %d is required.", b);
enum status {
    STATUS_OK,
    STATUS_BAD_ARGUMENT,
    STATUS_EMPTY_ARGUMENT,
    STATUS_EXTRA_CHARS,
    STATUS_RANGE_ERROR,
};

switch (status)
{
    case STATUS_BAD_ARGUMENT:
    case STATUS_EXTRA_CHARS:
        _throwerror(1, "%s is not a valid number.%s", s, b ? tmp : "");
    case STATUS_RANGE_ERROR:
        _throwerror(1, "%s is a too large number.", s);
}
int status;

if(!(status = strtoull(str, &end, base)) && (end == str))
    return 1;
if(!(*num = strtoull(str, &end, base)) && (end == str))
    return 1;

Context

StackExchange Code Review Q#102549, answer score: 7

Revisions (0)

No revisions yet.