6
\$\begingroup\$

There is a function that converts region code strings (1 to 4 characters and null terminator) to 32 bit integers codes to be used in maps as keys or values.

Blindly casting char* to int* is bad as it can be less than 4 bytes including null terminator.

Currently the code is like this

uint32_t region_code_key(const char* region_code) {
  unsigned char buf[4] = "\0";
  strncpy(buf, region_code, 4);
  return *((int*)buf);
}

I believe that buf may be not well aligned causing problems on some platforms. Is it a valid concern? The endianity is not a concern as such numbers are used only on local machines, only as keys.

It's a very simple function but If alignment concern is valid I see two ways to rewrite it. Here we just convert it byte by byte

uint32_t region_code_key(const char* region_code) {
    unsigned char* region_code_iter;
    unsigned char* region_code_end = region_code+4;
    uint32_t code_as_int = 0;
    for (region_code_iter = region_code; region_code_iter!=region_code_end && (*region_code_iter); ++region_code_iter) {
        code_as_int = (code_as_int<<8) | (*region_code_iter);
    }
    return code_as_int;
}

Alternatively use union to ensure better alignment:

uint32_t region_code_key(const char* region_code) {
union {
                char[sizeof(uint32_t)] as_string;
                uint32_t as_int;
} region = {0, 0, 0, 0};
strncpy(region.as_string, region_code, sizeof (region_code));
return region.as_int;
}

Is alignment a valid concern? If so which alternative seems less ugly to you?

\$\endgroup\$
2
\$\begingroup\$

Even if the char array is not aligned, your *((uint32_t *)buf) will be correct: the compiler will ensure the necessary operations are performed.

Note that I used unit32_t: the C standard does not guarantee the size of an int.

One thing: since these are 4 byte integers, you can use an endianness-independent way of doing things by using htonl()/ntohl().

\$\endgroup\$
  • 1
    \$\begingroup\$ On some microprocessors, an unaligned access is not legal, even though you get away with it on x86. \$\endgroup\$ – microtherion May 29 '13 at 11:00
  • \$\begingroup\$ For such microprocessors however, the compiler will do the right thing anyway, right? ;) \$\endgroup\$ – fge May 29 '13 at 11:01
  • 1
    \$\begingroup\$ For a procedure with a single parameter, as shown here, the compiler/runtime would take care to align the stack frame, so the code would work. However, the compilers I’ve seen will not generate special code for the cast as such, they will assume the programmer knew buf was aligned properly, and if it was not, the program will crash. \$\endgroup\$ – microtherion May 29 '13 at 11:09
  • \$\begingroup\$ On some systems the cast *((uint32_t *)buf) when buf is not properly aligned will lead to a crash \$\endgroup\$ – martinkunev Feb 27 '15 at 15:16
2
\$\begingroup\$

The buf array is on the stack. The compiler is not going to put misaligned variables on the stack. The stack frame created on a function call will put CPU registers on the stack, so if your processor has 32-bit registers (or greater), your first solution is okay for uint32_t alignment.

Note that the size in the 3rd solution strcpy is wrong:

strncpy(region.as_string, region_code, sizeof (region_code));

should be

strncpy(region.as_string, region_code, sizeof region);

For what it is worth, here is another solution :-)

static uint32_t region_code_key(const char *region_code)
{
    const uint32_t mask[] = {0, 0xff, 0xffff, 0xffffff, 0xffffffff};
    size_t len = strlen(region_code);
    if (len > 4) {
        len = 4;
    }
    const uint32_t code =
        (uint32_t)region_code[0]       |
        (uint32_t)region_code[1] << 8  |
        (uint32_t)region_code[2] << 16 |
        (uint32_t)region_code[3] << 24;
    return code & mask[len];
}
\$\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.