I am working through Kernighan and Ritchie to brush up on my C skills, so I thought I would invite critiques of my code.
Here is my Hex-to-int function:
int htoi(char *str, unsigned long long *result)
{
int c;
int i;
int msnibble;
int digits;
const int nibbles[] = {'\x0','\x1','\x2','\x3', \
'\x4','\x5','\x6','\x7', \
'\x8','\x9','\xA','\xB', \
'\xC','\xD','\xE','\xF'};
if (NULL == result) {
printf("Invalid pointer\n");
return -1;
}
if ('x' == str[1] || 'X' == str[1]) {
msnibble = 2;
digits = strlen(str) - 2;
} else {
msnibble = 0;
digits = strlen(str);
}
if (digits > 16) {
printf("Too many hex digits\n");
return -1;
}
for (i=0, *result = 0; '\0' != (c = str[i+msnibble]); i++) {
if ( c >= 'a' && c <= 'f') {
c = c - 'a' + 10;
} else if (c >= 'A' && c <= 'F') {
c = c - 'A' + 10;
} else if (c >= '0' && c <= '9') {
c -= '0';
} else {
printf("Non Hex Character Detected; Exiting\n");
return -1;
}
*result = *result << 4 | nibbles[c];
}
return 0;
}
I also tried a solution using the math library. Using pow, I would get correct output until I passed 0xffffffffffffffff and then it would be one greater than the correct answer. This is why I settled on using the array of nibbles.
Edit 1:
int htoi(const char *str, unsigned long long *result)
{
int c;
int i;
int msnibble;
int digits;
if (NULL == result) {
printf("Invalid pointer\n");
return -1;
}
if ('0' == str[0]) {
if ('x' == str[1] || 'X' == str[1]) {
msnibble = 2;
digits = strlen(str) - 2;
} else {
msnibble = 0;
digits = strlen(str);
}
}
if (digits > 16) {
printf("Too many hex digits\n");
return -1;
} else if (0 == digits)
return -1;
for (i=0, *result = 0; '\0' != (c = str[i+msnibble]); i++) {
if ( c >= 'a' && c <= 'f') {
c = c - 'a' + 10;
} else if (c >= 'A' && c <= 'F') {
c = c - 'A' + 10;
} else if (c >= '0' && c <= '9') {
c -= '0';
} else {
printf("Non hex character detected; Exiting\n");
return -1;
}
*result = *result << 4 | c;
}
return 0;
}
Edit 2:
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
enum status { NOINPUT = -1,
INVPTR = -2,
TOOBIG = -3,
NOHEX = -4,
NONHEX = -5};
int htoi(const char *str, unsigned long long *result);
int main(int argc, char *argv[])
{
unsigned long long result;
if (argc <= 1) {
printf("No input!\n");
return EXIT_FAILURE;
}
if (0 > htoi(argv[1],&result))
return EXIT_FAILURE;
else
printf("Value = %llu, strtol says %lu\n", result, strtol(argv[1],NULL,16));
return EXIT_SUCCESS;
}
int htoi(const char *str, unsigned long long *result)
{
int c;
int i;
int msnibble = 0;
int digits = 0;
if (NULL == result) {
printf("Invalid pointer\n");
return INVPTR;
}
if ('0' == str[0] && 'x' == str[1] || 'X' == str[1]) {
msnibble = 2;
digits = strlen(str) - 2;
} else {
msnibble = 0;
digits = strlen(str);
}
if (digits > 16) {
printf("Too many hex digits\n");
return TOOBIG;
} else if (0 == digits) {
printf("Nothing to do\n");
return NOHEX;
}
for (i=0, *result = 0; '\0' != (c = str[i+msnibble]); i++) {
if ( c >= 'a' && c <= 'f')
c = c - 'a' + 10;
else if (c >= 'A' && c <= 'F')
c = c - 'A' + 10;
else if (c >= '0' && c <= '9')
c -= '0';
else {
printf("Non hex character detected; Exiting\n");
return NONHEX;
}
*result = *result << 4 | c;
}
return EXIT_SUCCESS;
}
Edit 3:
I decided to simplify the error reporting and get rid of the printf
s. I also fixed the precedence error. I am omitting the braces on the if
s because I am following this. I am trying to follow the kernel style. Next time, I will include that with my post. I apologize for any confusion. I also got rid of i
and msnibble
as the pointer arithmetic is much simpler.
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
int htoi(const char *str, unsigned long *result);
int main(int argc, char *argv[])
{
unsigned long result;
if (argc <= 1) {
printf("No input!\n");
return EXIT_FAILURE;
}
if (0 > htoi(argv[1],&result))
return EXIT_FAILURE;
else
printf("Value = %llu, strtol says %lu\n", result, strtol(argv[1],NULL,16));
return EXIT_SUCCESS;
}
int htoi(const char *str, unsigned long *result)
{
int c;
int digits;
if ('0' == str[0] && ('x' == str[1] || 'X' == str[1]))
str += 2;
digits = strlen(str);
if (digits > sizeof(long)*2 || 0 == digits)
return -1;
for (*result = 0; '\0' != (c = *str); str++) {
if ( c >= 'a' && c <= 'f')
c = c - 'a' + 10;
else if (c >= 'A' && c <= 'F')
c = c - 'A' + 10;
else if (c >= '0' && c <= '9')
c -= '0';
else
return -1;
*result = *result << 4 | c;
}
return 0;
}
Edit 4:
#include <stdlib.h>
#include <limits.h>
#include <errno.h>
int htoi(const char *s, unsigned long *res);
int main(int argc, char *argv[])
{
unsigned long res;
if (argc <= 1) {
printf("No input!\n");
return EXIT_FAILURE;
}
if (0 > htoi(argv[1],&res))
return EXIT_FAILURE;
else
printf("Value = %llu, strtol says %lu\n", res, strtol(argv[1],NULL,16));
return EXIT_SUCCESS;
}
int htoi(const char *s, unsigned long *res)
{
int n;
int digits;
if ('0' == s[0] && ('x' == s[1] || 'X' == s[1]))
s += 2;
for (*res = 0; '\0' != *s; s++) {
n = 0;
if ( *s >= 'a' && *s <= 'f') {
n = *s - 'a' + 10;
} else if (*s >= 'A' && *s <= 'F') {
n = *s - 'A' + 10;
} else if (*s >= '0' && *s <= '9') {
n = *s - '0';
} else {
errno = EINVAL;
return -1;
}
if (*res > (ULONG_MAX/16)) {
errno = ERANGE;
return -1;
}
*res *= 16;
*res += (unsigned long) n;
}
return 0;
}
Edit 5:
#include <stdlib.h>
#include <limits.h>
#include <errno.h>
int htoi(const char *s, unsigned long *res);
int main(int argc, char *argv[])
{
unsigned long res;
if (argc <= 1) {
printf("No input!\n");
return EXIT_FAILURE;
}
if (0 > htoi(argv[1],&res))
return EXIT_FAILURE;
else
printf("Value = %llu, strtol says %lu\n", res, strtol(argv[1],NULL,16));
return EXIT_SUCCESS;
}
int htoi(const char *s, unsigned long *res)
{
if ('0' == s[0] && ('x' == s[1] || 'X' == s[1]))
s += 2;
int c;
unsigned long rc;
for (rc = 0; '\0' != (c = *s); s++) {
if ( c >= 'a' && c <= 'f') {
c = c - 'a' + 10;
} else if (c >= 'A' && c <= 'F') {
c = c - 'A' + 10;
} else if (c >= '0' && c <= '9') {
c = c - '0';
} else {
errno = EINVAL;
return -1;
}
if (rc > (ULONG_MAX/16)) {
errno = ERANGE;
return -1;
}
rc *= 16;
rc += (unsigned long) c;
}
*res = rc;
return 0;
}
strtol
can accomplish this. \$\endgroup\$-
in front of the macros. Also be sure to define theenum
somewhere here, otherwise the above code won't compile. You should also useEXIT_SUCCESS
instead, which is part of<stdlib.h>
. \$\endgroup\$