Hex string to byte array in the defined order
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
-1
down vote
favorite
Here is the function to convert hex string to byte array in the defined order. Any improvements?
#define CHECKINVALID 0
size_t FromHex(const char *dump, void *buff, int order) // order: 1 - normal, -1 reverse
uint8_t *cpos;
char *digit;
int cnibble;
size_t len = 0;
#if (CHECKINVALID == 1)
int n1, n2;
#endif
if (buff && dump)
if (len = strlen(dump))
cpos = order == -1 ? (uint8_t *)buff + len / 2 - !(len & 1) : buff;
if (len & 1)
#if (CHECKINVALID == 1)
if ((n1 = GetNum(*dump++)) == -1) return 0;
*cpos = n1;
#else
*cpos = GetNum(*dump++);
#endif
cpos += order;
while (*dump)
#if (CHECKINVALID == 1)
n1 = GetNum(*dump);
n2 = GetNum(*(dump + 1));
if (n1 == -1
return len / 2 + len & 1;
The example char to number conversion function is here: https://godbolt.org/g/MSSTn7 It optimizes very well on the systems using the ASCII table. It is __attribute__((noinline))
to make the compiled easier to read.
GetNum function:
int GetNum(const int n)
switch(n)
case '0':
return 0;
case '1':
return 1;
case '2':
return 2;
case '3':
return 3;
case '4':
return 4;
case '5':
return 5;
case '6':
return 6;
case '7':
return 7;
case '8':
return 8;
case '9':
return 9;
case 'A':
case 'a':
return 10;
case 'B':
case 'b':
return 11;
case 'C':
case 'c':
return 12;
case 'D':
case 'd':
return 13;
case 'E':
case 'e':
return 14;
case 'F':
case 'f':
return 15;
return -1;
it optimizes very efficiently and is not in the scope of the actual question.
Compiled code of the GetNum:
GetNum:
sub r0, r0, #48
cmp r0, #54
ldrls r3, .L5
ldrsbls r0, [r3, r0]
mvnhi r0, #0
bx lr
c serialization
add a comment |Â
up vote
-1
down vote
favorite
Here is the function to convert hex string to byte array in the defined order. Any improvements?
#define CHECKINVALID 0
size_t FromHex(const char *dump, void *buff, int order) // order: 1 - normal, -1 reverse
uint8_t *cpos;
char *digit;
int cnibble;
size_t len = 0;
#if (CHECKINVALID == 1)
int n1, n2;
#endif
if (buff && dump)
if (len = strlen(dump))
cpos = order == -1 ? (uint8_t *)buff + len / 2 - !(len & 1) : buff;
if (len & 1)
#if (CHECKINVALID == 1)
if ((n1 = GetNum(*dump++)) == -1) return 0;
*cpos = n1;
#else
*cpos = GetNum(*dump++);
#endif
cpos += order;
while (*dump)
#if (CHECKINVALID == 1)
n1 = GetNum(*dump);
n2 = GetNum(*(dump + 1));
if (n1 == -1
return len / 2 + len & 1;
The example char to number conversion function is here: https://godbolt.org/g/MSSTn7 It optimizes very well on the systems using the ASCII table. It is __attribute__((noinline))
to make the compiled easier to read.
GetNum function:
int GetNum(const int n)
switch(n)
case '0':
return 0;
case '1':
return 1;
case '2':
return 2;
case '3':
return 3;
case '4':
return 4;
case '5':
return 5;
case '6':
return 6;
case '7':
return 7;
case '8':
return 8;
case '9':
return 9;
case 'A':
case 'a':
return 10;
case 'B':
case 'b':
return 11;
case 'C':
case 'c':
return 12;
case 'D':
case 'd':
return 13;
case 'E':
case 'e':
return 14;
case 'F':
case 'f':
return 15;
return -1;
it optimizes very efficiently and is not in the scope of the actual question.
Compiled code of the GetNum:
GetNum:
sub r0, r0, #48
cmp r0, #54
ldrls r3, .L5
ldrsbls r0, [r3, r0]
mvnhi r0, #0
bx lr
c serialization
Any downvote comment?
â P__J__
May 14 at 18:14
2
I'm not the one who downvoted, but maybe if you edit your post to include how the function is used it would be better...
â Sam Onela
May 14 at 18:38
@TobySpeight followthe link to the godbolt. It is just hex digit to the number
â P__J__
May 15 at 11:05
add a comment |Â
up vote
-1
down vote
favorite
up vote
-1
down vote
favorite
Here is the function to convert hex string to byte array in the defined order. Any improvements?
#define CHECKINVALID 0
size_t FromHex(const char *dump, void *buff, int order) // order: 1 - normal, -1 reverse
uint8_t *cpos;
char *digit;
int cnibble;
size_t len = 0;
#if (CHECKINVALID == 1)
int n1, n2;
#endif
if (buff && dump)
if (len = strlen(dump))
cpos = order == -1 ? (uint8_t *)buff + len / 2 - !(len & 1) : buff;
if (len & 1)
#if (CHECKINVALID == 1)
if ((n1 = GetNum(*dump++)) == -1) return 0;
*cpos = n1;
#else
*cpos = GetNum(*dump++);
#endif
cpos += order;
while (*dump)
#if (CHECKINVALID == 1)
n1 = GetNum(*dump);
n2 = GetNum(*(dump + 1));
if (n1 == -1
return len / 2 + len & 1;
The example char to number conversion function is here: https://godbolt.org/g/MSSTn7 It optimizes very well on the systems using the ASCII table. It is __attribute__((noinline))
to make the compiled easier to read.
GetNum function:
int GetNum(const int n)
switch(n)
case '0':
return 0;
case '1':
return 1;
case '2':
return 2;
case '3':
return 3;
case '4':
return 4;
case '5':
return 5;
case '6':
return 6;
case '7':
return 7;
case '8':
return 8;
case '9':
return 9;
case 'A':
case 'a':
return 10;
case 'B':
case 'b':
return 11;
case 'C':
case 'c':
return 12;
case 'D':
case 'd':
return 13;
case 'E':
case 'e':
return 14;
case 'F':
case 'f':
return 15;
return -1;
it optimizes very efficiently and is not in the scope of the actual question.
Compiled code of the GetNum:
GetNum:
sub r0, r0, #48
cmp r0, #54
ldrls r3, .L5
ldrsbls r0, [r3, r0]
mvnhi r0, #0
bx lr
c serialization
Here is the function to convert hex string to byte array in the defined order. Any improvements?
#define CHECKINVALID 0
size_t FromHex(const char *dump, void *buff, int order) // order: 1 - normal, -1 reverse
uint8_t *cpos;
char *digit;
int cnibble;
size_t len = 0;
#if (CHECKINVALID == 1)
int n1, n2;
#endif
if (buff && dump)
if (len = strlen(dump))
cpos = order == -1 ? (uint8_t *)buff + len / 2 - !(len & 1) : buff;
if (len & 1)
#if (CHECKINVALID == 1)
if ((n1 = GetNum(*dump++)) == -1) return 0;
*cpos = n1;
#else
*cpos = GetNum(*dump++);
#endif
cpos += order;
while (*dump)
#if (CHECKINVALID == 1)
n1 = GetNum(*dump);
n2 = GetNum(*(dump + 1));
if (n1 == -1
return len / 2 + len & 1;
The example char to number conversion function is here: https://godbolt.org/g/MSSTn7 It optimizes very well on the systems using the ASCII table. It is __attribute__((noinline))
to make the compiled easier to read.
GetNum function:
int GetNum(const int n)
switch(n)
case '0':
return 0;
case '1':
return 1;
case '2':
return 2;
case '3':
return 3;
case '4':
return 4;
case '5':
return 5;
case '6':
return 6;
case '7':
return 7;
case '8':
return 8;
case '9':
return 9;
case 'A':
case 'a':
return 10;
case 'B':
case 'b':
return 11;
case 'C':
case 'c':
return 12;
case 'D':
case 'd':
return 13;
case 'E':
case 'e':
return 14;
case 'F':
case 'f':
return 15;
return -1;
it optimizes very efficiently and is not in the scope of the actual question.
Compiled code of the GetNum:
GetNum:
sub r0, r0, #48
cmp r0, #54
ldrls r3, .L5
ldrsbls r0, [r3, r0]
mvnhi r0, #0
bx lr
c serialization
edited May 15 at 17:18
Toby Speight
17.4k13488
17.4k13488
asked May 14 at 17:58
P__J__
1104
1104
Any downvote comment?
â P__J__
May 14 at 18:14
2
I'm not the one who downvoted, but maybe if you edit your post to include how the function is used it would be better...
â Sam Onela
May 14 at 18:38
@TobySpeight followthe link to the godbolt. It is just hex digit to the number
â P__J__
May 15 at 11:05
add a comment |Â
Any downvote comment?
â P__J__
May 14 at 18:14
2
I'm not the one who downvoted, but maybe if you edit your post to include how the function is used it would be better...
â Sam Onela
May 14 at 18:38
@TobySpeight followthe link to the godbolt. It is just hex digit to the number
â P__J__
May 15 at 11:05
Any downvote comment?
â P__J__
May 14 at 18:14
Any downvote comment?
â P__J__
May 14 at 18:14
2
2
I'm not the one who downvoted, but maybe if you edit your post to include how the function is used it would be better...
â Sam Onela
May 14 at 18:38
I'm not the one who downvoted, but maybe if you edit your post to include how the function is used it would be better...
â Sam Onela
May 14 at 18:38
@TobySpeight followthe link to the godbolt. It is just hex digit to the number
â P__J__
May 15 at 11:05
@TobySpeight followthe link to the godbolt. It is just hex digit to the number
â P__J__
May 15 at 11:05
add a comment |Â
1 Answer
1
active
oldest
votes
up vote
0
down vote
Missing headers
I had no definition of size_t
or uint8_t
until I added
#include <stddef.h>
#include <stdint.h>
and only implicit definition of strlen()
before adding
#include <string.h>
Warnings
We need some casts to eliminate these two warnings:
194385.c:52:25: warning: conversion from âÂÂintâ to âÂÂuint8_tâ aka âÂÂunsigned charâ may change value [-Wconversion]
*cpos = GetNum(*dump++);
^~~~~~
194385.c:64:25: warning: conversion from âÂÂintâ to âÂÂuint8_tâ aka âÂÂunsigned charâ may change value [-Wconversion]
*cpos = GetNum(*dump) * 16 + GetNum(*(dump + 1));
The other two warnings are easily dealt with, simply by not declaring digit
or cnibble
.
Dangerous interface
When implementing functions that write to user-supplied memory, it's generally a good idea to pass the size of that memory to the function (in the style of strncat()
, unlike strcat()
, for example). Perhaps you know that the function can never be called with more input than than catered for by the caller, but that's not clear in the review context, and every caller would need to be checked.
By analogy with standard functions, a safer and more consistent interface would be
enum hex_dir
reverse = -1,
forward = 1
;
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
I put the output buffer first, just like snprintf()
or memcpy()
, to help users call it correctly.
There's another aspect of the interface that's hard to use: the return type is naturally zero if given an empty string, however, we also overload that to indicate an error (in which case some of the output buffer may have been written). That's not an ideal interface design - perhaps consider reserving SIZE_MAX
for error return?
Missing tests
It's hard to be sure what the intended functionality is when the tests aren't included as part of the review. I had to make up my own tests to determine how it actually works:
#include <inttypes.h>
#include <stdarg.h>
#include <stdio.h>
#define EXPECT(...) expect(__FILE__, __LINE__, __VA_ARGS__)
int expect(const char *filename, int line_no,
size_t expected, size_t buf_size,
const char *input, enum hex_dir direction,
...)
uint8_t output[128];
if (buf_size > sizeof output)
fprintf(stderr, "%s:%d: Test limit exceeded (%zu > %zu) - increase buffer sizen",
filename, line_no, buf_size, sizeof output);
return 1;
int errors = 0;
size_t actual = FromHex(output, buf_size, input, direction);
if (actual != expected)
fprintf(stderr, "%s:%d: got result %zu, but expected %zu from input %sn",
filename, line_no, actual, expected, input);
++errors;
va_list args;
va_start(args, direction);
for (size_t i = 0; i < expected; ++i)
int n = va_arg(args, int);
if (n != output[i])
fprintf(stderr, "%s:%d: result[%zu] was %#" PRIx8", but should be %#xn",
filename, line_no, i, output[i], n);
++errors;
va_end(args);
return errors;
int main()
return
+ EXPECT(0, 1, "", forward)
+ EXPECT(0, 1, "", reverse)
+ EXPECT(1, 1, "0", forward, 0)
+ EXPECT(1, 1, "1", reverse, 1)
+ EXPECT(1, 1, "12", forward, 0x12)
+ EXPECT(1, 1, "12", reverse, 0x12)
+ EXPECT(2, 2, "123", forward, 0x01, 0x23)
+ EXPECT(2, 2, "123", reverse, 0x23, 0x01)
+ EXPECT(2, 2, "1234", forward, 0x12, 0x34)
+ EXPECT(2, 2, "1234", reverse, 0x34, 0x12)
/* insufficient buffer space */
+ EXPECT(1, 1, "123", forward, 0x01, 0x23)
+ EXPECT(1, 1, "123", reverse, 0x23, 0x01)
;
This did reveal the first bug (so I'm guessing your tests are even less thorough):
Be careful with operator precedence
This doesn't return what you think it does:
return len / 2 + len & 1;
I think you meant
return len / 2 + (len & 1);
or simply
return (len+1) / 2;
Implementation
Now we have some tests in place, let's look at the implementation.
The tests for CHECKINVALID == 1
are unusual. Generally, this sort of on/off compilation flag is controlled simply by whether or not it's defined, i.e. using #ifdef
. Moreover, it's risky to duplicate the functional code within the testing option; instead of
#ifdef CHECKINVALID
if ((n1 = GetNum(*dump++)) == -1) return 0;
*cpos = (uint8_t)n1;
#else
*cpos = (uint8_t)GetNum(*dump++);
#endif
I'd put only the check within the #ifdef
:
n1 = GetNum(*dump++);
#ifdef CHECKINVALID
if (n1 == -1) return 0;
#endif
*cpos = (uint8_t)n1;
Other changes that will make your code clearer are to move the variable declarations closer to where they are used, and to return quickly from the argument checks instead of having the body of the code in deeply nested if
s. Another improvement is to calculate the output length once, early on, since it's needed when initialising cpos
.
The resulting function is more readable IMO (and the tests ensure I haven't changed the functionality):
#ifdef CHECKINVALID
#define VERIFY(x) if (x == -1) return 0
#else
#define VERIFY(x) (void)0
#endif
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
!dump)
return 0;
if (direction * direction != 1)
return 0;
size_t len = strlen(dump);
if (len == 0)
return 0;
size_t out_len = (len+1) / 2;
if (out_len > buf_size)
return 0;
uint8_t *cpos = buff;
if (direction < 0)
/* reverse direction - start filling from end */
cpos += out_len - 1;
if (len & 1)
int n = GetNum(*dump++);
VERIFY(n);
*cpos = (uint8_t)n;
cpos += direction;
while (*dump)
int n1 = GetNum(*dump++);
int n2 = GetNum(*dump++);
VERIFY(n1);
VERIFY(n2);
*cpos = (uint8_t)(n1 * 16 + n2);
cpos += direction;
return out_len;
#undef VERIFY
Full program
#include <stddef.h>
#include <stdint.h>
#include <string.h>
/* define this to enable digit validation */
//#define CHECKINVALID
int GetNum(const char n)
switch(n)
case '0': return 0;
case '1': return 1;
case '2': return 2;
case '3': return 3;
case '4': return 4;
case '5': return 5;
case '6': return 6;
case '7': return 7;
case '8': return 8;
case '9': return 9;
case 'A': case 'a': return 0xa;
case 'B': case 'b': return 0xb;
case 'C': case 'c': return 0xc;
case 'D': case 'd': return 0xd;
case 'E': case 'e': return 0xe;
case 'F': case 'f': return 0xf;
return -1;
enum hex_dir
reverse = -1, /* write output from end */
forward = 1 /* write output from beginning */
;
#ifdef CHECKINVALID
#define VERIFY(x) if (x == -1) return 0
#else
#define VERIFY(x) (void)0
#endif
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
!dump)
return 0;
if (direction * direction != 1)
return 0;
size_t len = strlen(dump);
if (len == 0)
return 0;
size_t out_len = (len+1) / 2;
if (out_len > buf_size)
return 0;
uint8_t *cpos = buff;
if (direction < 0)
/* reverse direction - start filling from end */
cpos += out_len - 1;
if (len & 1)
int n = GetNum(*dump++);
VERIFY(n);
*cpos = (uint8_t)n;
cpos += direction;
while (*dump)
int n1 = GetNum(*dump++);
int n2 = GetNum(*dump++);
VERIFY(n1);
VERIFY(n2);
*cpos = (uint8_t)(n1 * 16 + n2);
cpos += direction;
return out_len;
#undef VERIFY
// --- TESTS
#include <inttypes.h>
#include <stdarg.h>
#include <stdio.h>
#define EXPECT(...) expect(__FILE__, __LINE__, __VA_ARGS__)
int expect(const char *filename, int line_no,
size_t expected, size_t buf_size,
const char *input, enum hex_dir direction,
...)
uint8_t output[128];
if (buf_size > sizeof output)
fprintf(stderr, "%s:%d: Test limit exceeded (%zu > %zu) - increase buffer sizen",
filename, line_no, buf_size, sizeof output);
return 1;
int errors = 0;
size_t actual = FromHex(output, buf_size, input, direction);
if (actual != expected)
fprintf(stderr, "%s:%d: got result %zu, but expected %zu from input %sn",
filename, line_no, actual, expected, input);
++errors;
va_list args;
va_start(args, direction);
for (size_t i = 0; i < expected; ++i)
int n = va_arg(args, int);
if (n != output[i])
fprintf(stderr, "%s:%d: result[%zu] was %#" PRIx8", but should be %#xn",
filename, line_no, i, output[i], n);
++errors;
va_end(args);
return errors;
int main()
return
+ EXPECT(0, 1, "", forward)
+ EXPECT(0, 1, "", reverse)
+ EXPECT(1, 1, "0", forward, 0)
+ EXPECT(1, 1, "1", reverse, 1)
+ EXPECT(1, 1, "f", forward, 0xf)
+ EXPECT(1, 1, "12", forward, 0x12)
+ EXPECT(1, 1, "12", reverse, 0x12)
+ EXPECT(2, 2, "123", forward, 0x01, 0x23)
+ EXPECT(2, 2, "123", reverse, 0x23, 0x01)
+ EXPECT(2, 2, "1234", forward, 0x12, 0x34)
+ EXPECT(2, 2, "1234", reverse, 0x34, 0x12)
#ifdef CHECKINVALID
+ EXPECT(0, 2, "g", reverse)
#endif
/* insufficient buffer space */
+ EXPECT(0, 1, "123", forward)
+ EXPECT(0, 1, "123", reverse)
;
add a comment |Â
1 Answer
1
active
oldest
votes
1 Answer
1
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
0
down vote
Missing headers
I had no definition of size_t
or uint8_t
until I added
#include <stddef.h>
#include <stdint.h>
and only implicit definition of strlen()
before adding
#include <string.h>
Warnings
We need some casts to eliminate these two warnings:
194385.c:52:25: warning: conversion from âÂÂintâ to âÂÂuint8_tâ aka âÂÂunsigned charâ may change value [-Wconversion]
*cpos = GetNum(*dump++);
^~~~~~
194385.c:64:25: warning: conversion from âÂÂintâ to âÂÂuint8_tâ aka âÂÂunsigned charâ may change value [-Wconversion]
*cpos = GetNum(*dump) * 16 + GetNum(*(dump + 1));
The other two warnings are easily dealt with, simply by not declaring digit
or cnibble
.
Dangerous interface
When implementing functions that write to user-supplied memory, it's generally a good idea to pass the size of that memory to the function (in the style of strncat()
, unlike strcat()
, for example). Perhaps you know that the function can never be called with more input than than catered for by the caller, but that's not clear in the review context, and every caller would need to be checked.
By analogy with standard functions, a safer and more consistent interface would be
enum hex_dir
reverse = -1,
forward = 1
;
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
I put the output buffer first, just like snprintf()
or memcpy()
, to help users call it correctly.
There's another aspect of the interface that's hard to use: the return type is naturally zero if given an empty string, however, we also overload that to indicate an error (in which case some of the output buffer may have been written). That's not an ideal interface design - perhaps consider reserving SIZE_MAX
for error return?
Missing tests
It's hard to be sure what the intended functionality is when the tests aren't included as part of the review. I had to make up my own tests to determine how it actually works:
#include <inttypes.h>
#include <stdarg.h>
#include <stdio.h>
#define EXPECT(...) expect(__FILE__, __LINE__, __VA_ARGS__)
int expect(const char *filename, int line_no,
size_t expected, size_t buf_size,
const char *input, enum hex_dir direction,
...)
uint8_t output[128];
if (buf_size > sizeof output)
fprintf(stderr, "%s:%d: Test limit exceeded (%zu > %zu) - increase buffer sizen",
filename, line_no, buf_size, sizeof output);
return 1;
int errors = 0;
size_t actual = FromHex(output, buf_size, input, direction);
if (actual != expected)
fprintf(stderr, "%s:%d: got result %zu, but expected %zu from input %sn",
filename, line_no, actual, expected, input);
++errors;
va_list args;
va_start(args, direction);
for (size_t i = 0; i < expected; ++i)
int n = va_arg(args, int);
if (n != output[i])
fprintf(stderr, "%s:%d: result[%zu] was %#" PRIx8", but should be %#xn",
filename, line_no, i, output[i], n);
++errors;
va_end(args);
return errors;
int main()
return
+ EXPECT(0, 1, "", forward)
+ EXPECT(0, 1, "", reverse)
+ EXPECT(1, 1, "0", forward, 0)
+ EXPECT(1, 1, "1", reverse, 1)
+ EXPECT(1, 1, "12", forward, 0x12)
+ EXPECT(1, 1, "12", reverse, 0x12)
+ EXPECT(2, 2, "123", forward, 0x01, 0x23)
+ EXPECT(2, 2, "123", reverse, 0x23, 0x01)
+ EXPECT(2, 2, "1234", forward, 0x12, 0x34)
+ EXPECT(2, 2, "1234", reverse, 0x34, 0x12)
/* insufficient buffer space */
+ EXPECT(1, 1, "123", forward, 0x01, 0x23)
+ EXPECT(1, 1, "123", reverse, 0x23, 0x01)
;
This did reveal the first bug (so I'm guessing your tests are even less thorough):
Be careful with operator precedence
This doesn't return what you think it does:
return len / 2 + len & 1;
I think you meant
return len / 2 + (len & 1);
or simply
return (len+1) / 2;
Implementation
Now we have some tests in place, let's look at the implementation.
The tests for CHECKINVALID == 1
are unusual. Generally, this sort of on/off compilation flag is controlled simply by whether or not it's defined, i.e. using #ifdef
. Moreover, it's risky to duplicate the functional code within the testing option; instead of
#ifdef CHECKINVALID
if ((n1 = GetNum(*dump++)) == -1) return 0;
*cpos = (uint8_t)n1;
#else
*cpos = (uint8_t)GetNum(*dump++);
#endif
I'd put only the check within the #ifdef
:
n1 = GetNum(*dump++);
#ifdef CHECKINVALID
if (n1 == -1) return 0;
#endif
*cpos = (uint8_t)n1;
Other changes that will make your code clearer are to move the variable declarations closer to where they are used, and to return quickly from the argument checks instead of having the body of the code in deeply nested if
s. Another improvement is to calculate the output length once, early on, since it's needed when initialising cpos
.
The resulting function is more readable IMO (and the tests ensure I haven't changed the functionality):
#ifdef CHECKINVALID
#define VERIFY(x) if (x == -1) return 0
#else
#define VERIFY(x) (void)0
#endif
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
!dump)
return 0;
if (direction * direction != 1)
return 0;
size_t len = strlen(dump);
if (len == 0)
return 0;
size_t out_len = (len+1) / 2;
if (out_len > buf_size)
return 0;
uint8_t *cpos = buff;
if (direction < 0)
/* reverse direction - start filling from end */
cpos += out_len - 1;
if (len & 1)
int n = GetNum(*dump++);
VERIFY(n);
*cpos = (uint8_t)n;
cpos += direction;
while (*dump)
int n1 = GetNum(*dump++);
int n2 = GetNum(*dump++);
VERIFY(n1);
VERIFY(n2);
*cpos = (uint8_t)(n1 * 16 + n2);
cpos += direction;
return out_len;
#undef VERIFY
Full program
#include <stddef.h>
#include <stdint.h>
#include <string.h>
/* define this to enable digit validation */
//#define CHECKINVALID
int GetNum(const char n)
switch(n)
case '0': return 0;
case '1': return 1;
case '2': return 2;
case '3': return 3;
case '4': return 4;
case '5': return 5;
case '6': return 6;
case '7': return 7;
case '8': return 8;
case '9': return 9;
case 'A': case 'a': return 0xa;
case 'B': case 'b': return 0xb;
case 'C': case 'c': return 0xc;
case 'D': case 'd': return 0xd;
case 'E': case 'e': return 0xe;
case 'F': case 'f': return 0xf;
return -1;
enum hex_dir
reverse = -1, /* write output from end */
forward = 1 /* write output from beginning */
;
#ifdef CHECKINVALID
#define VERIFY(x) if (x == -1) return 0
#else
#define VERIFY(x) (void)0
#endif
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
!dump)
return 0;
if (direction * direction != 1)
return 0;
size_t len = strlen(dump);
if (len == 0)
return 0;
size_t out_len = (len+1) / 2;
if (out_len > buf_size)
return 0;
uint8_t *cpos = buff;
if (direction < 0)
/* reverse direction - start filling from end */
cpos += out_len - 1;
if (len & 1)
int n = GetNum(*dump++);
VERIFY(n);
*cpos = (uint8_t)n;
cpos += direction;
while (*dump)
int n1 = GetNum(*dump++);
int n2 = GetNum(*dump++);
VERIFY(n1);
VERIFY(n2);
*cpos = (uint8_t)(n1 * 16 + n2);
cpos += direction;
return out_len;
#undef VERIFY
// --- TESTS
#include <inttypes.h>
#include <stdarg.h>
#include <stdio.h>
#define EXPECT(...) expect(__FILE__, __LINE__, __VA_ARGS__)
int expect(const char *filename, int line_no,
size_t expected, size_t buf_size,
const char *input, enum hex_dir direction,
...)
uint8_t output[128];
if (buf_size > sizeof output)
fprintf(stderr, "%s:%d: Test limit exceeded (%zu > %zu) - increase buffer sizen",
filename, line_no, buf_size, sizeof output);
return 1;
int errors = 0;
size_t actual = FromHex(output, buf_size, input, direction);
if (actual != expected)
fprintf(stderr, "%s:%d: got result %zu, but expected %zu from input %sn",
filename, line_no, actual, expected, input);
++errors;
va_list args;
va_start(args, direction);
for (size_t i = 0; i < expected; ++i)
int n = va_arg(args, int);
if (n != output[i])
fprintf(stderr, "%s:%d: result[%zu] was %#" PRIx8", but should be %#xn",
filename, line_no, i, output[i], n);
++errors;
va_end(args);
return errors;
int main()
return
+ EXPECT(0, 1, "", forward)
+ EXPECT(0, 1, "", reverse)
+ EXPECT(1, 1, "0", forward, 0)
+ EXPECT(1, 1, "1", reverse, 1)
+ EXPECT(1, 1, "f", forward, 0xf)
+ EXPECT(1, 1, "12", forward, 0x12)
+ EXPECT(1, 1, "12", reverse, 0x12)
+ EXPECT(2, 2, "123", forward, 0x01, 0x23)
+ EXPECT(2, 2, "123", reverse, 0x23, 0x01)
+ EXPECT(2, 2, "1234", forward, 0x12, 0x34)
+ EXPECT(2, 2, "1234", reverse, 0x34, 0x12)
#ifdef CHECKINVALID
+ EXPECT(0, 2, "g", reverse)
#endif
/* insufficient buffer space */
+ EXPECT(0, 1, "123", forward)
+ EXPECT(0, 1, "123", reverse)
;
add a comment |Â
up vote
0
down vote
Missing headers
I had no definition of size_t
or uint8_t
until I added
#include <stddef.h>
#include <stdint.h>
and only implicit definition of strlen()
before adding
#include <string.h>
Warnings
We need some casts to eliminate these two warnings:
194385.c:52:25: warning: conversion from âÂÂintâ to âÂÂuint8_tâ aka âÂÂunsigned charâ may change value [-Wconversion]
*cpos = GetNum(*dump++);
^~~~~~
194385.c:64:25: warning: conversion from âÂÂintâ to âÂÂuint8_tâ aka âÂÂunsigned charâ may change value [-Wconversion]
*cpos = GetNum(*dump) * 16 + GetNum(*(dump + 1));
The other two warnings are easily dealt with, simply by not declaring digit
or cnibble
.
Dangerous interface
When implementing functions that write to user-supplied memory, it's generally a good idea to pass the size of that memory to the function (in the style of strncat()
, unlike strcat()
, for example). Perhaps you know that the function can never be called with more input than than catered for by the caller, but that's not clear in the review context, and every caller would need to be checked.
By analogy with standard functions, a safer and more consistent interface would be
enum hex_dir
reverse = -1,
forward = 1
;
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
I put the output buffer first, just like snprintf()
or memcpy()
, to help users call it correctly.
There's another aspect of the interface that's hard to use: the return type is naturally zero if given an empty string, however, we also overload that to indicate an error (in which case some of the output buffer may have been written). That's not an ideal interface design - perhaps consider reserving SIZE_MAX
for error return?
Missing tests
It's hard to be sure what the intended functionality is when the tests aren't included as part of the review. I had to make up my own tests to determine how it actually works:
#include <inttypes.h>
#include <stdarg.h>
#include <stdio.h>
#define EXPECT(...) expect(__FILE__, __LINE__, __VA_ARGS__)
int expect(const char *filename, int line_no,
size_t expected, size_t buf_size,
const char *input, enum hex_dir direction,
...)
uint8_t output[128];
if (buf_size > sizeof output)
fprintf(stderr, "%s:%d: Test limit exceeded (%zu > %zu) - increase buffer sizen",
filename, line_no, buf_size, sizeof output);
return 1;
int errors = 0;
size_t actual = FromHex(output, buf_size, input, direction);
if (actual != expected)
fprintf(stderr, "%s:%d: got result %zu, but expected %zu from input %sn",
filename, line_no, actual, expected, input);
++errors;
va_list args;
va_start(args, direction);
for (size_t i = 0; i < expected; ++i)
int n = va_arg(args, int);
if (n != output[i])
fprintf(stderr, "%s:%d: result[%zu] was %#" PRIx8", but should be %#xn",
filename, line_no, i, output[i], n);
++errors;
va_end(args);
return errors;
int main()
return
+ EXPECT(0, 1, "", forward)
+ EXPECT(0, 1, "", reverse)
+ EXPECT(1, 1, "0", forward, 0)
+ EXPECT(1, 1, "1", reverse, 1)
+ EXPECT(1, 1, "12", forward, 0x12)
+ EXPECT(1, 1, "12", reverse, 0x12)
+ EXPECT(2, 2, "123", forward, 0x01, 0x23)
+ EXPECT(2, 2, "123", reverse, 0x23, 0x01)
+ EXPECT(2, 2, "1234", forward, 0x12, 0x34)
+ EXPECT(2, 2, "1234", reverse, 0x34, 0x12)
/* insufficient buffer space */
+ EXPECT(1, 1, "123", forward, 0x01, 0x23)
+ EXPECT(1, 1, "123", reverse, 0x23, 0x01)
;
This did reveal the first bug (so I'm guessing your tests are even less thorough):
Be careful with operator precedence
This doesn't return what you think it does:
return len / 2 + len & 1;
I think you meant
return len / 2 + (len & 1);
or simply
return (len+1) / 2;
Implementation
Now we have some tests in place, let's look at the implementation.
The tests for CHECKINVALID == 1
are unusual. Generally, this sort of on/off compilation flag is controlled simply by whether or not it's defined, i.e. using #ifdef
. Moreover, it's risky to duplicate the functional code within the testing option; instead of
#ifdef CHECKINVALID
if ((n1 = GetNum(*dump++)) == -1) return 0;
*cpos = (uint8_t)n1;
#else
*cpos = (uint8_t)GetNum(*dump++);
#endif
I'd put only the check within the #ifdef
:
n1 = GetNum(*dump++);
#ifdef CHECKINVALID
if (n1 == -1) return 0;
#endif
*cpos = (uint8_t)n1;
Other changes that will make your code clearer are to move the variable declarations closer to where they are used, and to return quickly from the argument checks instead of having the body of the code in deeply nested if
s. Another improvement is to calculate the output length once, early on, since it's needed when initialising cpos
.
The resulting function is more readable IMO (and the tests ensure I haven't changed the functionality):
#ifdef CHECKINVALID
#define VERIFY(x) if (x == -1) return 0
#else
#define VERIFY(x) (void)0
#endif
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
!dump)
return 0;
if (direction * direction != 1)
return 0;
size_t len = strlen(dump);
if (len == 0)
return 0;
size_t out_len = (len+1) / 2;
if (out_len > buf_size)
return 0;
uint8_t *cpos = buff;
if (direction < 0)
/* reverse direction - start filling from end */
cpos += out_len - 1;
if (len & 1)
int n = GetNum(*dump++);
VERIFY(n);
*cpos = (uint8_t)n;
cpos += direction;
while (*dump)
int n1 = GetNum(*dump++);
int n2 = GetNum(*dump++);
VERIFY(n1);
VERIFY(n2);
*cpos = (uint8_t)(n1 * 16 + n2);
cpos += direction;
return out_len;
#undef VERIFY
Full program
#include <stddef.h>
#include <stdint.h>
#include <string.h>
/* define this to enable digit validation */
//#define CHECKINVALID
int GetNum(const char n)
switch(n)
case '0': return 0;
case '1': return 1;
case '2': return 2;
case '3': return 3;
case '4': return 4;
case '5': return 5;
case '6': return 6;
case '7': return 7;
case '8': return 8;
case '9': return 9;
case 'A': case 'a': return 0xa;
case 'B': case 'b': return 0xb;
case 'C': case 'c': return 0xc;
case 'D': case 'd': return 0xd;
case 'E': case 'e': return 0xe;
case 'F': case 'f': return 0xf;
return -1;
enum hex_dir
reverse = -1, /* write output from end */
forward = 1 /* write output from beginning */
;
#ifdef CHECKINVALID
#define VERIFY(x) if (x == -1) return 0
#else
#define VERIFY(x) (void)0
#endif
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
!dump)
return 0;
if (direction * direction != 1)
return 0;
size_t len = strlen(dump);
if (len == 0)
return 0;
size_t out_len = (len+1) / 2;
if (out_len > buf_size)
return 0;
uint8_t *cpos = buff;
if (direction < 0)
/* reverse direction - start filling from end */
cpos += out_len - 1;
if (len & 1)
int n = GetNum(*dump++);
VERIFY(n);
*cpos = (uint8_t)n;
cpos += direction;
while (*dump)
int n1 = GetNum(*dump++);
int n2 = GetNum(*dump++);
VERIFY(n1);
VERIFY(n2);
*cpos = (uint8_t)(n1 * 16 + n2);
cpos += direction;
return out_len;
#undef VERIFY
// --- TESTS
#include <inttypes.h>
#include <stdarg.h>
#include <stdio.h>
#define EXPECT(...) expect(__FILE__, __LINE__, __VA_ARGS__)
int expect(const char *filename, int line_no,
size_t expected, size_t buf_size,
const char *input, enum hex_dir direction,
...)
uint8_t output[128];
if (buf_size > sizeof output)
fprintf(stderr, "%s:%d: Test limit exceeded (%zu > %zu) - increase buffer sizen",
filename, line_no, buf_size, sizeof output);
return 1;
int errors = 0;
size_t actual = FromHex(output, buf_size, input, direction);
if (actual != expected)
fprintf(stderr, "%s:%d: got result %zu, but expected %zu from input %sn",
filename, line_no, actual, expected, input);
++errors;
va_list args;
va_start(args, direction);
for (size_t i = 0; i < expected; ++i)
int n = va_arg(args, int);
if (n != output[i])
fprintf(stderr, "%s:%d: result[%zu] was %#" PRIx8", but should be %#xn",
filename, line_no, i, output[i], n);
++errors;
va_end(args);
return errors;
int main()
return
+ EXPECT(0, 1, "", forward)
+ EXPECT(0, 1, "", reverse)
+ EXPECT(1, 1, "0", forward, 0)
+ EXPECT(1, 1, "1", reverse, 1)
+ EXPECT(1, 1, "f", forward, 0xf)
+ EXPECT(1, 1, "12", forward, 0x12)
+ EXPECT(1, 1, "12", reverse, 0x12)
+ EXPECT(2, 2, "123", forward, 0x01, 0x23)
+ EXPECT(2, 2, "123", reverse, 0x23, 0x01)
+ EXPECT(2, 2, "1234", forward, 0x12, 0x34)
+ EXPECT(2, 2, "1234", reverse, 0x34, 0x12)
#ifdef CHECKINVALID
+ EXPECT(0, 2, "g", reverse)
#endif
/* insufficient buffer space */
+ EXPECT(0, 1, "123", forward)
+ EXPECT(0, 1, "123", reverse)
;
add a comment |Â
up vote
0
down vote
up vote
0
down vote
Missing headers
I had no definition of size_t
or uint8_t
until I added
#include <stddef.h>
#include <stdint.h>
and only implicit definition of strlen()
before adding
#include <string.h>
Warnings
We need some casts to eliminate these two warnings:
194385.c:52:25: warning: conversion from âÂÂintâ to âÂÂuint8_tâ aka âÂÂunsigned charâ may change value [-Wconversion]
*cpos = GetNum(*dump++);
^~~~~~
194385.c:64:25: warning: conversion from âÂÂintâ to âÂÂuint8_tâ aka âÂÂunsigned charâ may change value [-Wconversion]
*cpos = GetNum(*dump) * 16 + GetNum(*(dump + 1));
The other two warnings are easily dealt with, simply by not declaring digit
or cnibble
.
Dangerous interface
When implementing functions that write to user-supplied memory, it's generally a good idea to pass the size of that memory to the function (in the style of strncat()
, unlike strcat()
, for example). Perhaps you know that the function can never be called with more input than than catered for by the caller, but that's not clear in the review context, and every caller would need to be checked.
By analogy with standard functions, a safer and more consistent interface would be
enum hex_dir
reverse = -1,
forward = 1
;
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
I put the output buffer first, just like snprintf()
or memcpy()
, to help users call it correctly.
There's another aspect of the interface that's hard to use: the return type is naturally zero if given an empty string, however, we also overload that to indicate an error (in which case some of the output buffer may have been written). That's not an ideal interface design - perhaps consider reserving SIZE_MAX
for error return?
Missing tests
It's hard to be sure what the intended functionality is when the tests aren't included as part of the review. I had to make up my own tests to determine how it actually works:
#include <inttypes.h>
#include <stdarg.h>
#include <stdio.h>
#define EXPECT(...) expect(__FILE__, __LINE__, __VA_ARGS__)
int expect(const char *filename, int line_no,
size_t expected, size_t buf_size,
const char *input, enum hex_dir direction,
...)
uint8_t output[128];
if (buf_size > sizeof output)
fprintf(stderr, "%s:%d: Test limit exceeded (%zu > %zu) - increase buffer sizen",
filename, line_no, buf_size, sizeof output);
return 1;
int errors = 0;
size_t actual = FromHex(output, buf_size, input, direction);
if (actual != expected)
fprintf(stderr, "%s:%d: got result %zu, but expected %zu from input %sn",
filename, line_no, actual, expected, input);
++errors;
va_list args;
va_start(args, direction);
for (size_t i = 0; i < expected; ++i)
int n = va_arg(args, int);
if (n != output[i])
fprintf(stderr, "%s:%d: result[%zu] was %#" PRIx8", but should be %#xn",
filename, line_no, i, output[i], n);
++errors;
va_end(args);
return errors;
int main()
return
+ EXPECT(0, 1, "", forward)
+ EXPECT(0, 1, "", reverse)
+ EXPECT(1, 1, "0", forward, 0)
+ EXPECT(1, 1, "1", reverse, 1)
+ EXPECT(1, 1, "12", forward, 0x12)
+ EXPECT(1, 1, "12", reverse, 0x12)
+ EXPECT(2, 2, "123", forward, 0x01, 0x23)
+ EXPECT(2, 2, "123", reverse, 0x23, 0x01)
+ EXPECT(2, 2, "1234", forward, 0x12, 0x34)
+ EXPECT(2, 2, "1234", reverse, 0x34, 0x12)
/* insufficient buffer space */
+ EXPECT(1, 1, "123", forward, 0x01, 0x23)
+ EXPECT(1, 1, "123", reverse, 0x23, 0x01)
;
This did reveal the first bug (so I'm guessing your tests are even less thorough):
Be careful with operator precedence
This doesn't return what you think it does:
return len / 2 + len & 1;
I think you meant
return len / 2 + (len & 1);
or simply
return (len+1) / 2;
Implementation
Now we have some tests in place, let's look at the implementation.
The tests for CHECKINVALID == 1
are unusual. Generally, this sort of on/off compilation flag is controlled simply by whether or not it's defined, i.e. using #ifdef
. Moreover, it's risky to duplicate the functional code within the testing option; instead of
#ifdef CHECKINVALID
if ((n1 = GetNum(*dump++)) == -1) return 0;
*cpos = (uint8_t)n1;
#else
*cpos = (uint8_t)GetNum(*dump++);
#endif
I'd put only the check within the #ifdef
:
n1 = GetNum(*dump++);
#ifdef CHECKINVALID
if (n1 == -1) return 0;
#endif
*cpos = (uint8_t)n1;
Other changes that will make your code clearer are to move the variable declarations closer to where they are used, and to return quickly from the argument checks instead of having the body of the code in deeply nested if
s. Another improvement is to calculate the output length once, early on, since it's needed when initialising cpos
.
The resulting function is more readable IMO (and the tests ensure I haven't changed the functionality):
#ifdef CHECKINVALID
#define VERIFY(x) if (x == -1) return 0
#else
#define VERIFY(x) (void)0
#endif
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
!dump)
return 0;
if (direction * direction != 1)
return 0;
size_t len = strlen(dump);
if (len == 0)
return 0;
size_t out_len = (len+1) / 2;
if (out_len > buf_size)
return 0;
uint8_t *cpos = buff;
if (direction < 0)
/* reverse direction - start filling from end */
cpos += out_len - 1;
if (len & 1)
int n = GetNum(*dump++);
VERIFY(n);
*cpos = (uint8_t)n;
cpos += direction;
while (*dump)
int n1 = GetNum(*dump++);
int n2 = GetNum(*dump++);
VERIFY(n1);
VERIFY(n2);
*cpos = (uint8_t)(n1 * 16 + n2);
cpos += direction;
return out_len;
#undef VERIFY
Full program
#include <stddef.h>
#include <stdint.h>
#include <string.h>
/* define this to enable digit validation */
//#define CHECKINVALID
int GetNum(const char n)
switch(n)
case '0': return 0;
case '1': return 1;
case '2': return 2;
case '3': return 3;
case '4': return 4;
case '5': return 5;
case '6': return 6;
case '7': return 7;
case '8': return 8;
case '9': return 9;
case 'A': case 'a': return 0xa;
case 'B': case 'b': return 0xb;
case 'C': case 'c': return 0xc;
case 'D': case 'd': return 0xd;
case 'E': case 'e': return 0xe;
case 'F': case 'f': return 0xf;
return -1;
enum hex_dir
reverse = -1, /* write output from end */
forward = 1 /* write output from beginning */
;
#ifdef CHECKINVALID
#define VERIFY(x) if (x == -1) return 0
#else
#define VERIFY(x) (void)0
#endif
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
!dump)
return 0;
if (direction * direction != 1)
return 0;
size_t len = strlen(dump);
if (len == 0)
return 0;
size_t out_len = (len+1) / 2;
if (out_len > buf_size)
return 0;
uint8_t *cpos = buff;
if (direction < 0)
/* reverse direction - start filling from end */
cpos += out_len - 1;
if (len & 1)
int n = GetNum(*dump++);
VERIFY(n);
*cpos = (uint8_t)n;
cpos += direction;
while (*dump)
int n1 = GetNum(*dump++);
int n2 = GetNum(*dump++);
VERIFY(n1);
VERIFY(n2);
*cpos = (uint8_t)(n1 * 16 + n2);
cpos += direction;
return out_len;
#undef VERIFY
// --- TESTS
#include <inttypes.h>
#include <stdarg.h>
#include <stdio.h>
#define EXPECT(...) expect(__FILE__, __LINE__, __VA_ARGS__)
int expect(const char *filename, int line_no,
size_t expected, size_t buf_size,
const char *input, enum hex_dir direction,
...)
uint8_t output[128];
if (buf_size > sizeof output)
fprintf(stderr, "%s:%d: Test limit exceeded (%zu > %zu) - increase buffer sizen",
filename, line_no, buf_size, sizeof output);
return 1;
int errors = 0;
size_t actual = FromHex(output, buf_size, input, direction);
if (actual != expected)
fprintf(stderr, "%s:%d: got result %zu, but expected %zu from input %sn",
filename, line_no, actual, expected, input);
++errors;
va_list args;
va_start(args, direction);
for (size_t i = 0; i < expected; ++i)
int n = va_arg(args, int);
if (n != output[i])
fprintf(stderr, "%s:%d: result[%zu] was %#" PRIx8", but should be %#xn",
filename, line_no, i, output[i], n);
++errors;
va_end(args);
return errors;
int main()
return
+ EXPECT(0, 1, "", forward)
+ EXPECT(0, 1, "", reverse)
+ EXPECT(1, 1, "0", forward, 0)
+ EXPECT(1, 1, "1", reverse, 1)
+ EXPECT(1, 1, "f", forward, 0xf)
+ EXPECT(1, 1, "12", forward, 0x12)
+ EXPECT(1, 1, "12", reverse, 0x12)
+ EXPECT(2, 2, "123", forward, 0x01, 0x23)
+ EXPECT(2, 2, "123", reverse, 0x23, 0x01)
+ EXPECT(2, 2, "1234", forward, 0x12, 0x34)
+ EXPECT(2, 2, "1234", reverse, 0x34, 0x12)
#ifdef CHECKINVALID
+ EXPECT(0, 2, "g", reverse)
#endif
/* insufficient buffer space */
+ EXPECT(0, 1, "123", forward)
+ EXPECT(0, 1, "123", reverse)
;
Missing headers
I had no definition of size_t
or uint8_t
until I added
#include <stddef.h>
#include <stdint.h>
and only implicit definition of strlen()
before adding
#include <string.h>
Warnings
We need some casts to eliminate these two warnings:
194385.c:52:25: warning: conversion from âÂÂintâ to âÂÂuint8_tâ aka âÂÂunsigned charâ may change value [-Wconversion]
*cpos = GetNum(*dump++);
^~~~~~
194385.c:64:25: warning: conversion from âÂÂintâ to âÂÂuint8_tâ aka âÂÂunsigned charâ may change value [-Wconversion]
*cpos = GetNum(*dump) * 16 + GetNum(*(dump + 1));
The other two warnings are easily dealt with, simply by not declaring digit
or cnibble
.
Dangerous interface
When implementing functions that write to user-supplied memory, it's generally a good idea to pass the size of that memory to the function (in the style of strncat()
, unlike strcat()
, for example). Perhaps you know that the function can never be called with more input than than catered for by the caller, but that's not clear in the review context, and every caller would need to be checked.
By analogy with standard functions, a safer and more consistent interface would be
enum hex_dir
reverse = -1,
forward = 1
;
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
I put the output buffer first, just like snprintf()
or memcpy()
, to help users call it correctly.
There's another aspect of the interface that's hard to use: the return type is naturally zero if given an empty string, however, we also overload that to indicate an error (in which case some of the output buffer may have been written). That's not an ideal interface design - perhaps consider reserving SIZE_MAX
for error return?
Missing tests
It's hard to be sure what the intended functionality is when the tests aren't included as part of the review. I had to make up my own tests to determine how it actually works:
#include <inttypes.h>
#include <stdarg.h>
#include <stdio.h>
#define EXPECT(...) expect(__FILE__, __LINE__, __VA_ARGS__)
int expect(const char *filename, int line_no,
size_t expected, size_t buf_size,
const char *input, enum hex_dir direction,
...)
uint8_t output[128];
if (buf_size > sizeof output)
fprintf(stderr, "%s:%d: Test limit exceeded (%zu > %zu) - increase buffer sizen",
filename, line_no, buf_size, sizeof output);
return 1;
int errors = 0;
size_t actual = FromHex(output, buf_size, input, direction);
if (actual != expected)
fprintf(stderr, "%s:%d: got result %zu, but expected %zu from input %sn",
filename, line_no, actual, expected, input);
++errors;
va_list args;
va_start(args, direction);
for (size_t i = 0; i < expected; ++i)
int n = va_arg(args, int);
if (n != output[i])
fprintf(stderr, "%s:%d: result[%zu] was %#" PRIx8", but should be %#xn",
filename, line_no, i, output[i], n);
++errors;
va_end(args);
return errors;
int main()
return
+ EXPECT(0, 1, "", forward)
+ EXPECT(0, 1, "", reverse)
+ EXPECT(1, 1, "0", forward, 0)
+ EXPECT(1, 1, "1", reverse, 1)
+ EXPECT(1, 1, "12", forward, 0x12)
+ EXPECT(1, 1, "12", reverse, 0x12)
+ EXPECT(2, 2, "123", forward, 0x01, 0x23)
+ EXPECT(2, 2, "123", reverse, 0x23, 0x01)
+ EXPECT(2, 2, "1234", forward, 0x12, 0x34)
+ EXPECT(2, 2, "1234", reverse, 0x34, 0x12)
/* insufficient buffer space */
+ EXPECT(1, 1, "123", forward, 0x01, 0x23)
+ EXPECT(1, 1, "123", reverse, 0x23, 0x01)
;
This did reveal the first bug (so I'm guessing your tests are even less thorough):
Be careful with operator precedence
This doesn't return what you think it does:
return len / 2 + len & 1;
I think you meant
return len / 2 + (len & 1);
or simply
return (len+1) / 2;
Implementation
Now we have some tests in place, let's look at the implementation.
The tests for CHECKINVALID == 1
are unusual. Generally, this sort of on/off compilation flag is controlled simply by whether or not it's defined, i.e. using #ifdef
. Moreover, it's risky to duplicate the functional code within the testing option; instead of
#ifdef CHECKINVALID
if ((n1 = GetNum(*dump++)) == -1) return 0;
*cpos = (uint8_t)n1;
#else
*cpos = (uint8_t)GetNum(*dump++);
#endif
I'd put only the check within the #ifdef
:
n1 = GetNum(*dump++);
#ifdef CHECKINVALID
if (n1 == -1) return 0;
#endif
*cpos = (uint8_t)n1;
Other changes that will make your code clearer are to move the variable declarations closer to where they are used, and to return quickly from the argument checks instead of having the body of the code in deeply nested if
s. Another improvement is to calculate the output length once, early on, since it's needed when initialising cpos
.
The resulting function is more readable IMO (and the tests ensure I haven't changed the functionality):
#ifdef CHECKINVALID
#define VERIFY(x) if (x == -1) return 0
#else
#define VERIFY(x) (void)0
#endif
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
!dump)
return 0;
if (direction * direction != 1)
return 0;
size_t len = strlen(dump);
if (len == 0)
return 0;
size_t out_len = (len+1) / 2;
if (out_len > buf_size)
return 0;
uint8_t *cpos = buff;
if (direction < 0)
/* reverse direction - start filling from end */
cpos += out_len - 1;
if (len & 1)
int n = GetNum(*dump++);
VERIFY(n);
*cpos = (uint8_t)n;
cpos += direction;
while (*dump)
int n1 = GetNum(*dump++);
int n2 = GetNum(*dump++);
VERIFY(n1);
VERIFY(n2);
*cpos = (uint8_t)(n1 * 16 + n2);
cpos += direction;
return out_len;
#undef VERIFY
Full program
#include <stddef.h>
#include <stdint.h>
#include <string.h>
/* define this to enable digit validation */
//#define CHECKINVALID
int GetNum(const char n)
switch(n)
case '0': return 0;
case '1': return 1;
case '2': return 2;
case '3': return 3;
case '4': return 4;
case '5': return 5;
case '6': return 6;
case '7': return 7;
case '8': return 8;
case '9': return 9;
case 'A': case 'a': return 0xa;
case 'B': case 'b': return 0xb;
case 'C': case 'c': return 0xc;
case 'D': case 'd': return 0xd;
case 'E': case 'e': return 0xe;
case 'F': case 'f': return 0xf;
return -1;
enum hex_dir
reverse = -1, /* write output from end */
forward = 1 /* write output from beginning */
;
#ifdef CHECKINVALID
#define VERIFY(x) if (x == -1) return 0
#else
#define VERIFY(x) (void)0
#endif
size_t FromHex(uint8_t *buff, size_t buf_size,
const char *dump, enum hex_dir direction)
!dump)
return 0;
if (direction * direction != 1)
return 0;
size_t len = strlen(dump);
if (len == 0)
return 0;
size_t out_len = (len+1) / 2;
if (out_len > buf_size)
return 0;
uint8_t *cpos = buff;
if (direction < 0)
/* reverse direction - start filling from end */
cpos += out_len - 1;
if (len & 1)
int n = GetNum(*dump++);
VERIFY(n);
*cpos = (uint8_t)n;
cpos += direction;
while (*dump)
int n1 = GetNum(*dump++);
int n2 = GetNum(*dump++);
VERIFY(n1);
VERIFY(n2);
*cpos = (uint8_t)(n1 * 16 + n2);
cpos += direction;
return out_len;
#undef VERIFY
// --- TESTS
#include <inttypes.h>
#include <stdarg.h>
#include <stdio.h>
#define EXPECT(...) expect(__FILE__, __LINE__, __VA_ARGS__)
int expect(const char *filename, int line_no,
size_t expected, size_t buf_size,
const char *input, enum hex_dir direction,
...)
uint8_t output[128];
if (buf_size > sizeof output)
fprintf(stderr, "%s:%d: Test limit exceeded (%zu > %zu) - increase buffer sizen",
filename, line_no, buf_size, sizeof output);
return 1;
int errors = 0;
size_t actual = FromHex(output, buf_size, input, direction);
if (actual != expected)
fprintf(stderr, "%s:%d: got result %zu, but expected %zu from input %sn",
filename, line_no, actual, expected, input);
++errors;
va_list args;
va_start(args, direction);
for (size_t i = 0; i < expected; ++i)
int n = va_arg(args, int);
if (n != output[i])
fprintf(stderr, "%s:%d: result[%zu] was %#" PRIx8", but should be %#xn",
filename, line_no, i, output[i], n);
++errors;
va_end(args);
return errors;
int main()
return
+ EXPECT(0, 1, "", forward)
+ EXPECT(0, 1, "", reverse)
+ EXPECT(1, 1, "0", forward, 0)
+ EXPECT(1, 1, "1", reverse, 1)
+ EXPECT(1, 1, "f", forward, 0xf)
+ EXPECT(1, 1, "12", forward, 0x12)
+ EXPECT(1, 1, "12", reverse, 0x12)
+ EXPECT(2, 2, "123", forward, 0x01, 0x23)
+ EXPECT(2, 2, "123", reverse, 0x23, 0x01)
+ EXPECT(2, 2, "1234", forward, 0x12, 0x34)
+ EXPECT(2, 2, "1234", reverse, 0x34, 0x12)
#ifdef CHECKINVALID
+ EXPECT(0, 2, "g", reverse)
#endif
/* insufficient buffer space */
+ EXPECT(0, 1, "123", forward)
+ EXPECT(0, 1, "123", reverse)
;
answered May 15 at 17:14
Toby Speight
17.4k13488
17.4k13488
add a comment |Â
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f194385%2fhex-string-to-byte-array-in-the-defined-order%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Any downvote comment?
â P__J__
May 14 at 18:14
2
I'm not the one who downvoted, but maybe if you edit your post to include how the function is used it would be better...
â Sam Onela
May 14 at 18:38
@TobySpeight followthe link to the godbolt. It is just hex digit to the number
â P__J__
May 15 at 11:05