Converting binary data to printable hex

Question

In this thread some one commented that the following code should only be used in 'toy' projects. Unfortunately he hasn't come back to say why it's not of production quality so I was hoping some one in the community may be able to either assure me the code is ok (because I quite like it) or identify what is wrong.

template< class T1, class T2>
void hexascii( T1& out, const T2& in )
{
    out.resize( in.size() * 2 );
    const char hexDigits[] = {'0', '1', '2', '3', '4', '5', '6', '7','8', '9', 'A', 'B', 'C', 'D', 'E', 'F'};
    T1::iterator outit = out.begin();
    for( T2::const_iterator it = in.begin(); it != in.end(); ++it )
    {
        *outit++ = hexDigits[*it >> 4];
        *outit++ = hexDigits[*it & 0xF];
    }
}

template<class T1, class T2>
void asciihex( T1& out, const T2& in )
{
    size_t size = in.size;
    assert( !(size % 2) );

    out.resize( size / 2 );
    T1::iterator outit = out.begin();
    for( T2::const_iterator it = in.begin(); it != in.end(); it += 2, ++outit )
    {
    *outit = ((( (*it > '9' ? *it - 0x07 : *it)  - 0x30) << 4) & 0x00f0) + 
                (((*(it+1) > '9' ? *(it+1) - 0x07 : *(it+1)) - 0x30) & 0x000f);
    }
}

Edit: Thanks for your help guys, you've made some big improvements. I've written functions in the two suggested styles from your answers. Some rough testing suggests the second method is marginally faster than the first, but IMO this is outweighed by the improved readability of the first.

template<class T1>
void asciihex2( T1& out, const std::string& in )
{
    dassert( sizeof(T1::value_type)==1 );
    size_t size = in.size();
assert( !(size % 2) );
    out.resize( size / 2 );
    T1::iterator outit = out.begin();
    for( size_t i = 0; i < in.size(); i += 2 )
    {
        int tmp;
        sscanf( in.c_str() + i, "%02X", &tmp );
        *outit++ = tmp;
    }
}

template<class T1>
void asciihex3( T1& out, const std::string& in )
{
    dassert( sizeof(T1::value_type)==1 );
    size_t size = in.size();
assert( !(size % 2) );
    out.resize( size / 2 );
    T1::iterator outit = out.begin();
const char hexDigits[] = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 
                          0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
                  0x0A, 0x0B, 0x0C, 0x0D, 0x0E, 0x0F};
for( std::string::const_iterator it = in.begin(); it != in.end(); it += 2, ++outit )
    {
    *outit = (hexDigits[(*it - 0x30) & 0x1f] << 4) + 
              hexDigits[((*(it+1) - 0x30) & 0x1f)];
    }
}

Some of the assumptions surronding this code: 1: They are not intended as a generic, but are used in an anonymous name space to translate data for a specific class. 2: The templating is required as two separate container types are being used (one being std::vector, the other a similar byte array type container from a third party library. 3: The purpose is to be able to convert binary data of indeterminate length into strings and back again (0x1234abcd <-> "1234abcd") 4: assert traps errors in both debug and release modes 5: by the time these functions are called the size of the string will already have been checked, assert is used to terminate processing if something serious has gone wrong 6: It needs some commenting

Any other ideas appreciated.

Answer 1

It seems like a lot of templated code to achieve very little, given you have direct hex conversion in the standard C scanf and printf functions. why bother?

Answer 2

My main comment about it is that it's very difficult to read.

Especially:

*outit = ((( (*it > '9' ? *it - 0x07 : *it)  - 0x30) << 4) & 0x00f0) + 
            (((*(it+1) > '9' ? *(it+1) - 0x07 : *(it+1)) - 0x30) & 0x000f)

It would take my brain a little while to grok that, and annoy me if I inherited the code.

Answer 3

What is it supposed to do? There is no well-known accepted meaning of hexascii or asciihex, so the names should change.

[edit] Converting from binary to hex notation should often not be called ascii..., as ascii is a 7-bit format.

Answer 4

I don't really object against it. It's generic (within limits), it uses consts, references where needed, etc... It lacks a bit of documentation, and the asciihex *outit assignment is not quite clear at first sight.

resize initializes the output's elements unnecessary (use reserve instead).

Maybe the genericity is somewhat too flexible: you can feed the algorithms with any datatype you like, while you should only give it hex numbers (not e.g. a vector of doubles)

And indeed, it may be a bit overkill, given the presence of good library functions.

Answer 5

What's wrong with

*outit = hexDigits[*it]

Why can't these two functions share a common list of hexDigits and eliminate the complex (and slow) calculation of an ASCII character?

Answer 6

• Code has assert statements instead of proper handling of an error condition (and if your assert is turned off, the code may blow up)

• for loop has dangerous double-increase of iterator (it+=2). Especially in case your assert did not fire. What happens when your iterator is already at the end and you ++ it?

• Code is templated, but what you're doing is simply converting characters to numbers or the other way round. It's cargo cult programming. You hope that the blessings of template programming will come upon you by using templates. You even tagged this as a template question although the template aspect is completely irrelevant in your functions.

• the *outit= line is too complicated.

• code reinvents the wheel. In a big way.

Answer 7

Some problems that I see:

This will work great if it is only used for an input container that stores 8 bit types - e.g. char or unsigned char. For example, the following code will fail if used with a 32 bit type whose value after the right shift is greater than 15 - recommend that you always use a mask to ensure that lookup index is always within range.

*outit++ = hexDigits[*it >> 4];

What is the expected behavior if you pass in a container containing unsigned longs - for this to be a generic class it should probably be able to handle the conversion of 32 bit numbers to hext strings also.

This only works when the input is a container - what if I just want to convert a single byte? A suggestion here is to refactor the code into a core function that can covert a single byte (hex=>ascii and ascii=>hex) and then provide additional functions to use this core function for coverting containers of bytes etc.

In asciihex(), bad things will happen if the size of the input container is not divisible by 2. The use of:

it != in.end(); it += 2

is dangerous since if the container size is not divisible by 2 then the increment by two will advance the iterator past the end of the container and the comparison against end() will never work. This is somewhat protected against via the assert call but assert can be compiled out (e.g. it is often compiled out in release builds) so it would be much better to make this an if statement.

Answer 8

Problems I spot:

hexascii does not check if sizeof(T2::value_type)==1

hexascii dereferences it twice, asciihex even more. There's no reason for this, as you can store the result. This means you can't use an istream_iterator.

asciihex needs a random iterator as input, because (it+1) and (it+=2) are used. The algorithm could work on a forward iterator if you use only (++it).

(*it > '9' ? *it - 0x07 : *it) - 0x30 can be simplified to *it - (*it > '9' ? 0x37 : 0x30) so there is only one unconditional subtraction left. Still, an array lookup would be more efficient. Subtract 0x30. '0' will become 0;'A' will become 0x11 and 'a' will become 0x31. Mask with 0x1f to make it case-insensitive, and you can do the resulting lookup in a char[0x20] without overflow risks. Non-hex chars will just give you weird values.

Answer 9

The reason I would consider it toy code is there is no error checking.

I could pass it two vector and it would happily try and do something and make a complete mess generating random gibberish.