For the purposes of this code review, I will use the standard library, but pretend that <cstddef> is <stddef.h>, that I didn't use std::cout, that new and delete[] are overloaded and call custom malloc functions, etc. So this is -std=c++11 -ffreestanding.

Here are my questions:

  • Currently my tohex function returns a buffer allocated on the heap. Is it better if I have the caller pass the buffer as a parameter instead? Is there a way to avoid heap allocation without using variable length arrays?
  • I have a separate function that counts the size of the digits so that I can reverse the string. What's an alternate way of doing this?
  • Should I prefer uint8_t or int or is unsigned okay? I don't think there should be any issues with what type I choose in the code shown.

#include <cstddef>

unsigned count(unsigned n)
{
    int digits = 0;
    while (n > 0)
    {
        ++digits;
        n /= 16;
    }
    return digits;
}

char* tohex(unsigned n)
{
    unsigned size = count(n) + 1;
    char *buffer = new char[size];
    unsigned index = size - 2;
    while (n > 0)
    {
        unsigned mod = n % 16;

        if (mod >= 10)
            buffer[index--] = (mod - 10) + 'A';
        else
            buffer[index--] = mod + '0';

        n /= 16;
    }
    buffer[size - 1] = '\0';
    return buffer;
}

#include <iostream>

int main()
{
    char *buffer = tohex(0xBEEFCAFE);
    std::cout << buffer;
    delete[] buffer;
    return 0;
}
  • More accurately, it converts a number to a hexadecimal string. – 200_success Sep 12 '14 at 9:07
  • There is a bug if you pass 0. count(0) will return 0. Luckily there is also a bug tohex(0) were it will print nothing. – Martin York Sep 12 '14 at 16:16

As both the others have said use std::string if you are going to dynamically allocate space. Its not going to be worse than your code;if your compiler implements the small string optimizations its actually goinf to do better than your code as no dynamic memory will be used.

unsigned count(unsigned n)

Well we know that unsigned have a particular size sizeof(unsigned). So we know that the max size of the array is sizeof(unsigned)*CHAR_BITS/4. On most systems this will be 8 on some it will be 16. But neither of these numbers is huge why not always use the maximum size (it will save you the time of calculating an exact size.

You can also get away with no dynamic memory allocation by having a static array in the function (not thread safe but it does not look like that is your issue).

Others have suggested replacing the n /= 16; by n >> 4;. DON'T do that (unless you think it expresses intent better). It hides the intent of the code. The code is supposed to express the intent in the most natural way possible. It is the job of the compiler to see that this can be optimized to a shift by 4 and I am dead sure the compiler will generate the most efficient code.

  • I see no reason to use char* over std::string here. The latter does the memory management for you (no need for new/delete). Whether or not this already leaks, using std::string should help eliminate this uncertainly.

  • Keep your #includes at the very top, where they can be easily maintained. The compiler will still have to compile all the library code, so it doesn't really help to move any somewhere else.

  • The name count() is misleading here as it just takes an int. Since it appears to return the number of digits in the argument, you should rename the function to digits().

For C++ it is always better to use class that manages the deallocation in destructor. This could be std::string or std::unique_ptr. For custom allocation/deallocation you should adapt class similar to std::allocator and use std::basic_string<char, std::char_traits<char>, my_allocator>. In case of std::unique_ptr, you can provide the Deleter. Ohter possibility would be to create your own wrapper similar to unique_ptr, but maybe with conversion operator const char* ().

You could pass the buffer into the function to make it thread-safe (not to return address of static internal buffer) and for this situation, I see no problem to have bigger buffer - it will still be quite small (9B).


Now few hints about optimization:

unsigned mod = n % 16;
n /= 16;

Division is slow and specifically these are better written as bit-masking and shifts:

unsigned mod = n & 15; // or: n & 0xF -- get/mask the nibble
n >>= 4;               // move to next nibble

unsigned vs. signed vs. other types: well, you better use unsigned not to run unto troubles with the signbit (sign-extension, sign-preserving shifts, division, modulo). But otherwise, it is your choice to use unsigned or uint32_t.

If the question was not about the argument (for tohex) but generic, then I would advise to use standard unsigned, int, char, unsigned char when used in the program, but uint8_t when you write some message-data-structure to be passed over IPC or packets (to make it architecture-independent). There are even big-endian and little-endian versions... but this seems to be far away from the simple example.

  • "There are even big-endian and little-endian versions" May you explain what you mean? There're functions which convert BE <==> LE. – edmz Sep 12 '14 at 7:44
  • 1
    @black: Well, these are not part of the standard (those least and fast have different meaning), but you may find e.g. htonl() and custom typedefs to signal little_uint32_t and bigendian_uint32_t or similar in such message/packet interfaces. – firda Sep 12 '14 at 8:05
  • 2
    The compiler has already worked out that division by 16 is a shift by 4 and planted the appropriate code to do that. Don't use shifting when you mean division it makes maintenance harder. Let the compiler do its job and do the peephole optimizations for you. – Martin York Sep 12 '14 at 16:14
  • Which compiler? GCC? CLang? Which versions? ....maybe I am still too low-level for using e.g. IAR (which is quite mature and well-optimizing as well) for embedded software (ARM Cortex). – firda Sep 12 '14 at 16:19
  • Does no matter. Its just a bad idea. – Martin York Sep 12 '14 at 16:30

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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