10
\$\begingroup\$

It's been a while since I've been properly grilled about my code.

I had this Idea which should probably never make it into production code but still I couldn't find anything seriously wrong with it at a glance...

All your comments are welcome.

#ifndef BITFIELDMEMBER_H
#define BITFIELDMEMBER_H

/************************************************************************
 * This is for creating Endianness-free bit-fields.
 * The values are always packed as big-endian.
 * When used for arithmetics/comparison, the values are read in the
 * proper endianness for the current CPU mode.
 *
 * Note: The fields can overlap if you want
 */

template<int firstBit, int bitSize>
struct BitFieldMember
{
    typedef BitFieldMember<firstBit, bitSize> self_t;
    typedef unsigned char uchar;
    enum {
        lastBit = firstBit + bitSize - 1,
        mask = (1 << bitSize) - 1
    };
    uchar *selfArray()
    {
        return reinterpret_cast<uchar *>(this);
    }
    const uchar *selfArray() const
    {
        return reinterpret_cast<const uchar *>(this);
    }

    /* used to read data from the field */
    /* will also work with all the operators that work with integral types */
    inline operator unsigned() const
    {
        const uchar *arr = selfArray();
        const uchar *p = arr + firstBit / 8;
        int i = 8 - (firstBit & 7);
        unsigned ret = 0;
        ret |= *p;
        while (i < bitSize)
        {
            ret <<= 8;
            ret |= *(++p);
            i += 8;
        }
        return ((ret >> (7 - (lastBit & 7))) & mask);
    }

    /* used to assign a value into the field */
    inline self_t& operator=(unsigned m)
    {
        uchar *arr = selfArray();
        m &= mask;
        unsigned wmask = ~(mask << (7 - (lastBit & 7)));
        m <<= (7 - (lastBit & 7));
        uchar *p = arr + lastBit / 8;
        int i = (lastBit & 7) + 1;
        (*p &= wmask) |= m;
        while (i < bitSize)
        {
            m >>= 8;
            wmask >>= 8;
            (*(--p) &= wmask) |= m;
            i += 8;
        }
        return *this;
    }

    inline self_t& operator+=(unsigned m)
    {
        *this = *this + m;
        return *this;
    }

    inline self_t& operator-=(unsigned m)
    {
        *this = *this - m;
        return *this;
    }

    inline self_t& operator*=(unsigned m)
    {
        *this = *this * m;
        return *this;
    }

    inline self_t& operator/=(unsigned m)
    {
        *this = *this / m;
        return *this;
    }

    inline self_t& operator%=(unsigned m)
    {
        *this = *this % m;
        return *this;
    }

    inline self_t& operator<<=(unsigned m)
    {
        *this = *this << m;
        return *this;
    }

    inline self_t& operator>>=(unsigned m)
    {
        *this = *this >> m;
        return *this;
    }

    inline self_t& operator|=(unsigned m)
    {
        *this = *this | m;
        return *this;
    }

    inline self_t& operator&=(unsigned m)
    {
        *this = *this & m;
        return *this;
    }

    inline self_t& operator^=(unsigned m)
    {
        *this = *this ^ m;
        return *this;
    }

};
#endif

Usage example:

union header
{
    unsigned char arr[2];       // space allocation, 2 bytes (16 bits)

    BitFieldMember<0, 4> m1;     // first 4 bits
    BitFieldMember<4, 5> m2;     // The following 5 bits
    BitFieldMember<9, 6> m3;     // The following 6 bits, total 16 bits
};

int main()
{
    header a;
    memset(a.arr, 0, sizeof(a.arr));
    a.m1 = rand();
    a.m3 = a.m1;
    a.m2 = ~a.m1;
    return 0;
}

This sometimes wouldn't work for fields that are larger than 24 bits... the shifting may trim the most significant bits.

\$\endgroup\$
  • \$\begingroup\$ there's a blog post with the same code here blog.codef00.com/2014/12/06/portable-bitfields-using-c11. Is this yours? It mentions UB, but does not specify where and what kind of UB that is. Could you probably tell more about that? \$\endgroup\$ – Slava Oct 18 '17 at 19:48
  • \$\begingroup\$ That one isn't me. I didn't read the blog post. What is UB? \$\endgroup\$ – CplusPuzzle Oct 18 '17 at 20:00
  • \$\begingroup\$ Undefined Behaviour. Funny, that blog post appeared at about the same time as yours, and the code is similar, though a bit different. \$\endgroup\$ – Slava Oct 18 '17 at 20:58
5
\$\begingroup\$

Your code is good, except for two methods : operator unsigned() const and self_t& operator=(unsigned m). They are too complex, and by just looking at them, it is far from clear what they are doing.

First, you can add new values to your enum :

enum {
    lastBit = firstBit + bitSize - 1,
    mask = (1 << bitSize) - 1,
    firstBitIndex = 8 - (firstBit & 7),                         // change to proper name
    nameThisValueProperly = (7 - (lastBit & 7)) & mask,         // change to proper name
};

First modify your operator unsigned() const to this :

inline operator unsigned() const
{
    const uchar *arr = selfArray();
    const uchar *p = arr + firstBit / 8;

    unsigned result = CalculateSomeValue( p );

    return (result >> nameThisValueProperly );
}

// name this method properly
inline unsigned CalculateSomeValue( const uchar *p ) const
{
    int i = firstBitIndex;
    unsigned result = *p;

    while (i < bitSize)
    {
        result <<= 8;
        result |= *(++p);
        i += 8;
    }

    return result;
}

Of course, you need to put proper names both for enum values and method names, so when someone looks into this code says "Aha, that is what is is doing". This way, you can easy increase code clarity.

Also, by making method smaller, you can easily test them.

More about this method, you can read in Clean code by Robert Martin.

You can do something similar for the self_t& operator=(unsigned m). Break it up, until you find it clean.

\$\endgroup\$
  • \$\begingroup\$ There's a small bug in there: nameThisValueProperly = (7 - (lastBit & 7)) & mask breaks things for field sizes 1 & 2. Removing &mask fixes it, but make sure there's a &mask wherever this value gets used. \$\endgroup\$ – Brian Vandenberg Apr 24 '18 at 0:59
4
\$\begingroup\$

Here are a few things found wrong with this implementation when using it in production code are the following:

Assignment between fields that are of the exact same type will result in an empty implementation with nothing copied because the fields have no storage of their own. To resolve this, it's best to cast the right side of the assignment to unsigned. The trouble is finding this bug in the first place.

A workaround would have been to block the assignment by either making it private or delete it with = delete and let the compiler find it for you but this will require C++11 or higher.

C++03 doesn't allow union members to be types that have declarations for assignment operators.

Also, it's missing some operators:

inline self_t& operator++()
{
    *this = *this + 1;
    return *this;
}

inline self_t& operator--()
{
    *this = *this - 1;
    return *this;
}

inline unsigned operator++(int)
{
    unsigned tmp = *this;
    *this = tmp + 1;
    return tmp;
}

inline unsigned operator--(int)
{
    unsigned tmp = *this;
    *this = tmp - 1;
    return tmp;
}

Last and least (assuming unsigned is 32 bits), if someone insists on having fields that are 32 bits, you will need to make the following modification to the enum of the mask:

enum
{
    lastBit = firstBit + bitSize - 1,
    mask = (1ULL << bitSize) - 1
};

Other than that, it served us well.

\$\endgroup\$
  • \$\begingroup\$ In case it wasn't clear, assignment between objects of the union type works fine because it's basically just a memcpy \$\endgroup\$ – CplusPuzzle Oct 19 '17 at 5:43
1
\$\begingroup\$

I implemented yours with the following changes:

  • Replaced enum with static constexpr
  • Deleted default / copy / move constructors
    • Constructors imply ownership of resources. I figured this would help avoid undefined behavior
  • Deleted unary operator&() for similar reasons.
  • Implemented self_t& operator=(const self_t&) to deal with the problem you noted:

    Assignment between fields that are of the exact same type will result in an empty implementation with nothing copied because the fields have no storage of their own.

    Originally I added an array, necessitating an extra template argument: BitfieldMember<sizeof(data), ...>, but this made it unnecessary.

  • Added the template argument from the last bullet and a static_assert in an attempt to require the storage be large enough.
  • Changed the set/get logic for readability:

private:
inline operator base_type() const {
  base_type ret = 0;
  for( unsigned ii = firstBit / 8; ii <= lastBit / 8; ++ii ) {
    ret = (ret << 8) | data()[ii];
  }
  return (ret >> excessBitsInLastByte) & mask;
}
inline self_t& operator=( base_type m ) {
  m = (m & mask) << excessBitsInLastByte;
  base_type write_mask = mask << excessBitsInLastByte;
  for( int ii = lastBit / 8; ii >= 0 && 0 != write_mask; --ii ) {
    uchar& ref = data()[ii];
    ref &= ~write_mask;
    ref |= m;
    m >>= 8;
    write_mask >>= 8;
  }
  return *this;
}

I've been looking for a way to add compile time errors for the following situations but haven't managed it yet:

  • if any fields overlap
    • I have some ideas for this, haven't tried them yet.
  • if the union's size is larger than its array
    • This works but I was hoping for something more automatic: put a static_assert after the union definition is complete.
  • if virtual functions are added to BitfieldMember (or derived classes have virtual functions)
    • The vtable will increase the union's size and each one will clobber each-other's vtable.
    • It's probably not possible to catch the latter case
  • If data members are added to BitfieldMember (or deriving classes)
    • I can catch the first case automatically, but I don't think it's possible to catch the 2nd case.
\$\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.