-1
\$\begingroup\$

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 || n2 == -1) return 0;
                *cpos = n1 * 16 + n2;
            #else
                *cpos = GetNum(*dump) * 16 + GetNum(*(dump + 1));
            #endif
                cpos += order;
                dump += 2;
            }
        }
    }
    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
\$\endgroup\$
  • \$\begingroup\$ Any downvote comment? \$\endgroup\$ – P__J__ May 14 '18 at 18:14
  • 2
    \$\begingroup\$ 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... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ May 14 '18 at 18:38
  • \$\begingroup\$ @TobySpeight followthe link to the godbolt. It is just hex digit to the number \$\endgroup\$ – P__J__ May 15 '18 at 11:05
1
\$\begingroup\$

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 size\n",
                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 %s\n",
                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 %#x\n",
                    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)
{
    if (!buff || !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)
{
    if (!buff || !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 size\n",
                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 %s\n",
                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 %#x\n",
                    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)
        ;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.