1
\$\begingroup\$

Will be grateful if anyone would reasonably comment on my code. I am interested in the style only. If there will be any comments which are not related to the style I am glad to read them as well.

   #include <iostream>

    unsigned reverse0(unsigned);

    union FourBytes
    {
        unsigned _value;
        struct Word
        {
            unsigned b1 : 8;
            unsigned b2 : 8;
            unsigned b3 : 8;
            unsigned b4 : 8;
        } _word;
    };
    unsigned reverse1(unsigned);

    int main(void)
    {
        int x(1), y(2);

        ++x = y;

        std::cout << reverse0(2447) << std::endl
            << reverse1(2447) << std::endl;
        return 0;
    }

    unsigned reverse0(unsigned p)
    {
        unsigned res(0);
        for (int i(0); i < sizeof(p)*8; ++i)
        {
            unsigned ithBit( (p & (1<<i)) >> i);
            int byteNumber(sizeof(p) - (i / 8) - 1);
            res |= (ithBit << (byteNumber*8+i%8));
        }
        return res;
    }

    unsigned reverse1(unsigned p)
    {
        FourBytes fb;
        fb._value = p;

        FourBytes res;
        res._word.b1 = fb._word.b4;
        res._word.b2 = fb._word.b3;
        res._word.b3 = fb._word.b2;
        res._word.b4 = fb._word.b1;

        return res._value;
    }
\$\endgroup\$
5
\$\begingroup\$

Style-wise, objectively improvable:

Avoid leaving dead code

This looks like junk you forgot to delete:

int x(1), y(2);

++x = y;

Comment non-trivial algorithms:

reverse0() is funky enough that I had to squint a bit and take a quick minute to run the code in my head to even have an idea of what it's trying to do. An explanation of what each line is "supposed" to be doing would have been helpful here.

Either name the struct type or the field name, not both

struct Word
{
    unsigned b1 : 8;
    unsigned b2 : 8;
    unsigned b3 : 8;
    unsigned b4 : 8;
} _word;

You don't need Word for anything, so you should just omit it entirely

Redundant explicit void argument list in main declaration:

int main(void) should just be int main()

Style-wise: opinions:

I don't like using unsigned

Since unsigned generally reads like a qualifier (I know it's not, it just looks like one), I personally much prefer being explicit and using unsigned int instead.

non-style stuff, but important:

don't use non-sized types with hard-coded sizeof() expectations.

Your FourBytes struct assumes that sizeof(unsigned) is 4, which is bad, you should be using std::uint32_t to properly enforce that.

Writing to an union field and reading from another one is illegal

This is surprising to many people as using unions to perform type punning is such a common technique, but it is, in fact, undefined behavior. You are only allowed to read from the last field you wrote to.

That means your reverse1() function is not valid C++ at all.

What's legal is reading and writing from a char (or unsigned char) array obtained from reinterpret_casting the pointer of a standard layout object. In fact, it's the only legal way to do direct memory manipulation of an existing object.

Edit: as requested, an implementation of reverse() using casting to char array:

unsigned int reverse(unsigned int v) {
   unsigned int result = 0;

   char const* src = reinterpret_cast<char const*>(&v);
   char* dst = reinterpret_cast<char*>(&result);

   for(int i = 0; i < sizeof(v); ++i) {
     dst[sizeof(v) - i - 1] = src[i];
   }

  return result;
}
\$\endgroup\$
  • \$\begingroup\$ Could you, please, provide a code which would reverse the bytes using the char type somehow? \$\endgroup\$ – trafalgarLaww Dec 3 '17 at 12:46
  • \$\begingroup\$ @trafalgarLaww Added an example of using casting. \$\endgroup\$ – Frank Dec 3 '17 at 17:14
2
\$\begingroup\$

Do not use union if you can achieve the same result using bitshifting.

uint32_t reverse_u32(uint32_t x) {
    return ((x & 0xff) << 24)
        | ((x & 0xff00) << 8)
        | ((x & 0xff0000) >> 8)
        | ((x & 0xff000000u) >> 24);
}

The compiler and optimizer will take care of the rest. They should produce a single instruction of code (bswap on x86) for the above code.

\$\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.