Hex string to byte array in the defined order

The name of the pictureThe name of the pictureThe name of the pictureClash 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






share|improve this question





















  • 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
















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






share|improve this question





















  • 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












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






share|improve this question













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








share|improve this question












share|improve this question




share|improve this question








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
















  • 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










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 ifs. 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)
;






share|improve this answer





















    Your Answer




    StackExchange.ifUsing("editor", function ()
    return StackExchange.using("mathjaxEditing", function ()
    StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
    StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
    );
    );
    , "mathjax-editing");

    StackExchange.ifUsing("editor", function ()
    StackExchange.using("externalEditor", function ()
    StackExchange.using("snippets", function ()
    StackExchange.snippets.init();
    );
    );
    , "code-snippets");

    StackExchange.ready(function()
    var channelOptions =
    tags: "".split(" "),
    id: "196"
    ;
    initTagRenderer("".split(" "), "".split(" "), channelOptions);

    StackExchange.using("externalEditor", function()
    // Have to fire editor after snippets, if snippets enabled
    if (StackExchange.settings.snippets.snippetsEnabled)
    StackExchange.using("snippets", function()
    createEditor();
    );

    else
    createEditor();

    );

    function createEditor()
    StackExchange.prepareEditor(
    heartbeatType: 'answer',
    convertImagesToLinks: false,
    noModals: false,
    showLowRepImageUploadWarning: true,
    reputationToPostImages: null,
    bindNavPrevention: true,
    postfix: "",
    onDemand: true,
    discardSelector: ".discard-answer"
    ,immediatelyShowMarkdownHelp:true
    );



    );








     

    draft saved


    draft discarded


















    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






























    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 ifs. 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)
    ;






    share|improve this answer

























      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 ifs. 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)
      ;






      share|improve this answer























        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 ifs. 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)
        ;






        share|improve this answer













        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 ifs. 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)
        ;







        share|improve this answer













        share|improve this answer



        share|improve this answer











        answered May 15 at 17:14









        Toby Speight

        17.4k13488




        17.4k13488






















             

            draft saved


            draft discarded


























             


            draft saved


            draft discarded














            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













































































            Popular posts from this blog

            Chat program with C++ and SFML

            Function to Return a JSON Like Objects Using VBA Collections and Arrays

            Will my employers contract hold up in court?