Problems with byte-reversing an integer

Question

Godday all.

Could someone please explain the logical difference in these two implementations of a byte-reversing function.

Example 1:

uint32_t byte_reverse_32(uint32_t num) {
    static union bytes {
        uint8_t b[4];
        uint32_t n;
    } bytes;
    bytes.n = num;

    uint32_t ret = 0;
    ret |= bytes.b[0] << 24;
    ret |= bytes.b[1] << 16;
    ret |= bytes.b[2] << 8;
    ret |= bytes.b[3];

    return ret;
}  

Example 2:

uint32_t byte_reverse_32(uint32_t num) {
    static union bytes {
        uint8_t b[4];
        uint32_t n;
    } bytes;
    bytes.n = num;

    uint32_t ret = 0;
    ret |= (bytes.b[0] << 24) || (bytes.b[1] << 16) || (bytes.b[2] << 8) || (bytes.b[3]);
    return ret;
}

I must be missing something because for the unsigned integer 0xFE12ADCF the first example correctly gives 0xCFAD12FE while the second one yields 1. What am I missing?

By the way, I couldn't figure out how to get '<<' inside the pre+code-tags, hence LSHIFT. If it's possible to do it, feel free to edit (and comment on how =)).

Thanks.

EDIT: Fixed the bytes-variable which was never assigned to.

Answer 1

You are using ||, which is a boolean operator, that is true (1), if one of the two arguments is different from zero.

To get correct results, you need the bitwise OR operator which is |, like you did in your first example (x |= y is x = x | y not x = x || y).

So the second example should work with

ret |= (bytes.b[0] LSHIFT 24) | (bytes.b[1] LSHIFT 16) | (bytes.b[2] LSHIFT 8) | (bytes.b[3]);

Hope that helps.

Answer 2

You are using || which is the logical or operator, you want the binary | operator.

Answer 3

| is not the same operator as ||. The first is a bitwise OR, which is what you want. The second is a boolean OR, which is what you have.

Answer 4

You're not using the num parameter at all, but instead you use the uninitialized bytes union variable.

A union is not really necessary here, you can simply cast the num parameter in a way that you can access its bytes:

uint8_t *b = (uint8_t*)#
// then access b[0] through b[3]    

And as the other guys already answered, use the bitwise | operator instead of ||.

Answer 5

The question was already answered, but just my 2 cents about a code that could enter production:

Using the static keyword in your code as below:

uint32_t byte_reverse_32(uint32_t num) {
    static union bytes {
        uint8_t b[4];
        uint32_t n;
    } bytes;
/* etc.*/

has no performance value (or any value anyway), and will make sure your function will have data races if called from multiple threads (i.e. it won't be safe to use).

Do remove the static: Your function will be as fast, and multithread safe (not mentioning simpler).

uint32_t byte_reverse_32(uint32_t num) {
    union bytes {
        uint8_t b[4];
        uint32_t n;
    } bytes;
/* etc.*/

Answer 6

It should be noted that the whole approach to byte-reversing an integer by mixing physical-level memory access and value-level bitwise operations looks highly suspect. I mean, it might work for you, but why would anyone do it that way? What was the point of creating such a weird mix?

If you decided to resort to direct physical memory access by re-interpreting the integer value as a sequence of 4 bytes, the natural reversal approach would be to just swap the bytes 0 and 3 and swap the bytes 1 and 2.

uint32_t byte_reverse_32(uint32_t num) {
    union bytes {
        uint8_t b[4];
        uint32_t n;
    } bytes;
    uint8_t t;

    bytes.n = num;

    t = bytes.b[0]; bytes.b[0] = bytes.b[3]; bytes.b[3] = t;
    t = bytes.b[1]; bytes.b[1] = bytes.b[2]; bytes.b[2] = t;

    return bytes.n;
}

Alternative approach would be to do the whole thing at the value-level by implementing everything through bitwise operations without any memory re-interpretation.

uint32_t byte_reverse_32(uint32_t num) {
    uint32_t res;

    res = num & 0xFF; num >>= 8; res <<= 8;
    res = num & 0xFF; num >>= 8; res <<= 8;
    res = num & 0xFF; num >>= 8; res <<= 8;
    res = num & 0xFF; 

    return res;
}

Both of the above approaches, if implemented correctly, would be portable in a sense that they would work on both big- and little-endian platforms. People would normally turn to value-level bitwise approach specifically to avoid physical memory re-interpretation through a union, since memory re-interpretation is almost always a hack.

But what you have now (i.e. both approaches mixed together) seems to make very little sense (if any at all). What you have now would perform the reversal on a little-endian platform, but would do nothing on a big-endian platform. I understand that you might not be interested in big-endian platforms at all, so the issue is not critical for you. But still the mix of the two alternative (and normally mutually exclusive) techniques looks rather bizarre to me.

Again, if you have already decided on using the union-based approach, just swap the bytes and be done with it.