4
\$\begingroup\$

Context:

I have some C code that uses a lot of bit-flags. The flag-sets may have a couple of hundred members. Currently these flags are defined as a 32-bit mask that must be applied to a specific element.

/* Flags: */
#define OF1_RESIST_FIRE      0x00000001L
#define OF1_RESIST_COLD      0x00000002L
#define OF1_RESIST_ACID      0x00000004L
...
#define OF2_SUSTAIN_STRENGTH 0x00000001L
...
#define OF9_AGGRAVATES_CATS  0x00000004L

/* Bitset: */
uint32_t oflags1;
uint32_t oflags2;
...

To make this easier to manage, I'm hoping to change the flags to be defined as bit indices instead, and consolidate the flags into an array, e.g.:

/* Flags: */
enum object_flags {
    RESIST_FIRE,
    RESIST_COLD,
    RESIST_ACID,
    ...
    SUSTAIN_STRENGTH,
    ...
    AGGRAVATES_CATS,
    ...
    MAX_OBJECT_FLAGS,
};

/* Bitset: */
BITSET_DECLARE(oflags, MAX_OBJECT_FLAGS); // uint32_t oflags[9];

(note: above code is illustrative for context, not for review).


Code:

For this purpose, I've created some macros to help manipulate the bitset array, based on this comp.lang.c faq item (bflags.h):

#pragma once

#include <stdint.h>
#include <limits.h>

/* 
 * Macros to simplify creating and using bit flags.
 *
 * Flags are stored in an array of uint32_t, the size of which
 * is determined by the required number of bits.
 *
 * Any bits after the required size may change during operation.
 */

/* The underlying type of the array */
#define BFLAGS_WORD_T uint32_t
/* The number of bits in the underlying type */
#define BFLAGS_WORD_BIT_SIZE (sizeof(BFLAGS_WORD_T) * CHAR_BIT)

/* The number of elements in the array */
#define BFLAGS_ARRAY_SIZE(bits) \
    (((bits) + BFLAGS_WORD_BIT_SIZE - 1) / BFLAGS_WORD_BIT_SIZE)

/* Get the array element containing this bit */
#define BFLAGS_WORD_INDEX(bit) ((bit) / BFLAGS_WORD_BIT_SIZE)

/* Create a mask for this bit in the array element that contains it */
#define BFLAGS_WORD_MASK(bit) (1 << ((bit) % BFLAGS_WORD_BIT_SIZE))


/* Declare a bit flag array containing enough elements to
 * store the requested number of bits. */
#define BFLAGS_DECLARE(bflags, bits) \
    BFLAGS_WORD_T bflags[BFLAGS_ARRAY_SIZE(bits)]

/* As BFLAGS_DECLARE, but also zeros the array contents.
 * Prefer to use this where possible. */
#define BFLAGS_DECLARE_ZERO(bflags, bits) \
    BFLAGS_WORD_T bflags[BFLAGS_ARRAY_SIZE(bits)] = { 0 }


/* Set all bits in the array to 0 */
#define BFLAGS_ZERO(bflags, bits) \
    do { \
        for (size_t i = 0; i != BFLAGS_ARRAY_SIZE(bits); ++i) \
            (bflags)[i] = (BFLAGS_WORD_T)0; \
    } while (false)

/* Set all bits in the array to 1 */
#define BFLAGS_FILL(bflags, bits) \
    do { \
        for (size_t i = 0; i != BFLAGS_ARRAY_SIZE(bits); ++i) \
            (bflags)[i] = ~(BFLAGS_WORD_T)0; \
    } while (false)


/* Copy to another array */
#define BFLAGS_COPY(bflags_out, bflags_a, bits) \
    do { \
        for (size_t i = 0; i != BFLAGS_ARRAY_SIZE(bits); ++i) \
            (bflags_out)[i] = (bflags_a)[i]; \
    } while (false)

/* Test two arrays for equality.
 * Bits of the last array element may differ after the last bit
 * so we mask them out before comparing. */
#define BFLAGS_EQ(eq_out, bflags_a, bflags_b, bits) \
    do { \
        eq_out = true; \
        for (size_t i = 0; i != BFLAGS_ARRAY_SIZE(bits) - 1; ++i) \
            eq_out &= ((bflags_a)[i] == (bflags_b)[i]); \
        BFLAGS_WORD_T mask = (~(BFLAGS_WORD_T)0) >> ((BFLAGS_WORD_BIT_SIZE - ((bits) % BFLAGS_WORD_BIT_SIZE)) % BFLAGS_WORD_BIT_SIZE); \
        eq_out &= (((bflags_a)[BFLAGS_ARRAY_SIZE(bits) - 1] & mask) == ((bflags_b)[BFLAGS_ARRAY_SIZE(bits) - 1] & mask)); \
    } while (false)


/* Set the specified bit to 1 */
#define BFLAGS_SET(bflags, bit) ((bflags)[BFLAGS_WORD_INDEX(bit)] |= BFLAGS_WORD_MASK(bit))

/* Set the specified bit to 0 */
#define BFLAGS_CLEAR(bflags, bit) ((bflags)[BFLAGS_WORD_INDEX(bit)] &= ~BFLAGS_WORD_MASK(bit))

/* Toggle the specified bit */
#define BFLAGS_FLIP(bflags, bit) ((bflags)[BFLAGS_WORD_INDEX(bit)] ^= BFLAGS_WORD_MASK(bit))

/* Check the value of the specified bit */
#define BFLAGS_TEST(bflags, bit) (((bflags)[BFLAGS_WORD_INDEX(bit)] & BFLAGS_WORD_MASK(bit)) != 0)


/* Do a bitwise-and of the specified bit flag arrays and put the result 
 * in bflags_out. All the flag sets should contain the same number of bits. */
#define BFLAGS_AND(bflags_out, bflags_a, bflags_b, bits) \
    do { \
        for (size_t i = 0; i != BFLAGS_ARRAY_SIZE(bits); ++i) \
            (bflags_out)[i] = (bflags_a)[i] & (bflags_b)[i]; \
    } while (false)

 /* Do a bitwise-or of the specified bit flag arrays and put the result
  * in bflags_out. All the flag sets should contain the same number of bits. */
#define BFLAGS_OR(bflags_out, bflags_a, bflags_b, bits) \
    do { \
        for (size_t i = 0; i != BFLAGS_ARRAY_SIZE(bits); ++i) \
            (bflags_out)[i] = (bflags_a)[i] | (bflags_b)[i]; \
    } while (false)

 /* Do a bitwise-xor of the specified bit flag arrays and put the result
  * in bflags_out. All the flag sets should contain the same number of bits. */
#define BFLAGS_XOR(bflags_out, bflags_a, bflags_b, bits) \
    do { \
        for (size_t i = 0; i != BFLAGS_ARRAY_SIZE(bits); ++i) \
            (bflags_out)[i] = (bflags_a)[i] ^ (bflags_b)[i]; \
    } while (false)

 /* Do a bitwise-not of the specified bit flag array and put the result
  * in bflags_out. Both the flag sets should contain the same number of bits. */
#define BFLAGS_NOT(bflags_out, bflags_a, bits) \
    do { \
        for (size_t i = 0; i != BFLAGS_ARRAY_SIZE(bits); ++i) \
            (bflags_out)[i] = ~(bflags_a)[i]; \
    } while (false)

Any feedback is welcome, but especially about:

  • Macro safety (missing parentheses? extra parentheses? anything else?)
  • Is there perhaps a way to "return" the result of BFLAGS_EQ from the macro, instead of passing it in?

Unit tests:

#include "bflags.h"

#include <assert.h>
#include <stdio.h>
#include <stdbool.h>


#define UNUSED(x) __attribute((unused)) x

#define mu_assert(test) do { if (!(test)) return #test; } while (0)
#define mu_run_test(test) do { char *message = test(); tests_run++; if (message) return message; } while (0)
extern int tests_run;


static char* bflags_test_declare()
{
    {
        //BFLAGS_DECLARE(bflags, 0); // won't compile (size 0 array)
    }
    {
        BFLAGS_DECLARE(bflags, 1);
        static_assert(sizeof(bflags) / sizeof(bflags[0]) == 1, "");
    }
    {
        BFLAGS_DECLARE(bflags, 32);
        static_assert(sizeof(bflags) / sizeof(bflags[0]) == 1, "");
    }
    {
        BFLAGS_DECLARE(bflags, 33);
        static_assert(sizeof(bflags) / sizeof(bflags[0]) == 2, "");
    }

    return NULL;
}

static char* bflags_test_declare_zero()
{
    {
        BFLAGS_DECLARE_ZERO(bflags, 1);
        mu_assert(bflags[0] == 0);
    }
    {
        BFLAGS_DECLARE_ZERO(bflags, 320);
        for (size_t i = 0; i != BFLAGS_ARRAY_SIZE(320); ++i)
            mu_assert(bflags[i] == 0);
    }

    return NULL;
}

static char* bflags_zero()
{
    {
        BFLAGS_DECLARE(bflags, 32);
        bflags[0] = 0xffffffff;

        BFLAGS_ZERO(bflags, 32);
        mu_assert(bflags[0] == 0);
    }
    {
        BFLAGS_DECLARE(bflags, 321);
        for (size_t i = 0; i != BFLAGS_ARRAY_SIZE(321); ++i)
            bflags[i] = (BFLAGS_WORD_T)i;

        mu_assert(bflags[1] == 1);

        BFLAGS_ZERO(bflags, 321);

        for (size_t i = 0; i != BFLAGS_ARRAY_SIZE(321); ++i)
            mu_assert(bflags[i] == 0);
    }

    return NULL;
}

static char* bflags_fill()
{
    {
        BFLAGS_DECLARE_ZERO(bflags, 32);

        BFLAGS_FILL(bflags, 32);
        mu_assert(bflags[0] == 0xffffffff);
    }
    {
        BFLAGS_DECLARE_ZERO(bflags, 35);

        BFLAGS_FILL(bflags, 35);
        mu_assert(bflags[0] == 0xffffffff);

        // values above bit size are also set, but we shouldn't depend on it!
        for (size_t i = 0; i != 35; ++i)
            mu_assert(BFLAGS_TEST(bflags, i) != 0);
    }

    return NULL;
}

static char* bflags_copy()
{
    {
        BFLAGS_DECLARE_ZERO(bflags_a, 59);

        for (size_t i = 0; i != 59; ++i)
            if (i % 6 == 0)
                BFLAGS_SET(bflags_a, i);

        BFLAGS_DECLARE_ZERO(bflags_b, 59);
        BFLAGS_COPY(bflags_b, bflags_a, 59);

        for (size_t i = 0; i != 59; ++i)
            if (i % 6 == 0)
                mu_assert(BFLAGS_TEST(bflags_b, i));
            else
                mu_assert(!BFLAGS_TEST(bflags_b, i));
    }

    return NULL;
}

static char* bflags_eq()
{
    {
        BFLAGS_DECLARE_ZERO(bflags_a, 32);
        BFLAGS_DECLARE_ZERO(bflags_b, 32);

        {
            bool eq = false;
            BFLAGS_EQ(eq, bflags_a, bflags_b, 32);
            mu_assert(eq);
        }

        BFLAGS_SET(bflags_a, 25);

        {
            bool eq = false;
            BFLAGS_EQ(eq, bflags_a, bflags_b, 32);
            mu_assert(!eq);
        }
    }

    {
        BFLAGS_DECLARE_ZERO(bflags_a, 59);

        for (size_t i = 0; i != 59; ++i)
            if (i % 6 == 0)
                BFLAGS_SET(bflags_a, i);

        BFLAGS_DECLARE_ZERO(bflags_b, 59);
        BFLAGS_COPY(bflags_b, bflags_a, 59);

        {
            bool eq = false;
            BFLAGS_EQ(eq, bflags_a, bflags_b, 59);
            mu_assert(eq);
        }

        BFLAGS_SET(bflags_a, 60); // set a bit after the end...

        {
            bool eq = false;
            BFLAGS_EQ(eq, bflags_a, bflags_b, 59);
            mu_assert(eq);
        }
    }

    return NULL;
}

static char* bflags_set()
{
    {
        BFLAGS_DECLARE_ZERO(bflags, 32);

        BFLAGS_SET(bflags, 0);
        mu_assert(bflags[0] == (1 << 0));

        BFLAGS_SET(bflags, 5);
        mu_assert(bflags[0] == ((1 << 0) | (1 << 5)));

        BFLAGS_SET(bflags, 31);
        mu_assert(bflags[0] == ((1 << 0) | (1 << 5) | (1 << 31)));
    }
    {
        BFLAGS_DECLARE_ZERO(bflags, 64);

        BFLAGS_SET(bflags, 0);
        mu_assert(bflags[0] == (1 << 0));

        BFLAGS_SET(bflags, 63);
        mu_assert(bflags[1] == (1 << 31));
    }
    {
        BFLAGS_DECLARE_ZERO(bflags, 32);

        BFLAGS_SET(bflags, 12);
        mu_assert(bflags[0] == (1 << 12));

        BFLAGS_SET(bflags, 12);
        mu_assert(bflags[0] == (1 << 12));
    }

    return NULL;
}

static char* bflags_clear()
{
    {
        BFLAGS_DECLARE_ZERO(bflags, 32);

        BFLAGS_FILL(bflags, 32);

        BFLAGS_CLEAR(bflags, 5);
        mu_assert(bflags[0] == ~(1 << 5));

        BFLAGS_CLEAR(bflags, 0);
        mu_assert(bflags[0] == ~((1 << 0) | (1 << 5)));
    }

    {
        BFLAGS_DECLARE_ZERO(bflags, 35);

        BFLAGS_FILL(bflags, 35);
        mu_assert(BFLAGS_TEST(bflags, 31));
        mu_assert(BFLAGS_TEST(bflags, 32));

        BFLAGS_CLEAR(bflags, 32);
        mu_assert(!BFLAGS_TEST(bflags, 32));
        mu_assert(bflags[0] == ~(BFLAGS_WORD_T)0);

        BFLAGS_CLEAR(bflags, 33);
        mu_assert(!BFLAGS_TEST(bflags, 33));
        mu_assert(bflags[0] == ~(BFLAGS_WORD_T)0);
    }

    {
        BFLAGS_DECLARE_ZERO(bflags, 32);

        BFLAGS_FILL(bflags, 32);

        BFLAGS_CLEAR(bflags, 16);
        mu_assert(bflags[0] == ~(1 << 16));

        BFLAGS_CLEAR(bflags, 16);
        mu_assert(!BFLAGS_TEST(bflags, 16));
    }

    return NULL;
}

static char* bflags_flip()
{
    {
        BFLAGS_DECLARE_ZERO(bflags, 32);

        BFLAGS_FLIP(bflags, 13);
        mu_assert(bflags[0] == (1 << 13));

        BFLAGS_FLIP(bflags, 13);
        mu_assert(bflags[0] == 0);
    }

    {
        BFLAGS_DECLARE_ZERO(bflags, 72);

        BFLAGS_FLIP(bflags, 68);
        mu_assert(bflags[0] == 0 && bflags[1] == 0);
        mu_assert(BFLAGS_TEST(bflags, 68));
    }

    return NULL;
}

static char* bflags_test()
{
    {
        BFLAGS_DECLARE_ZERO(bflags, 128);

        mu_assert(!BFLAGS_TEST(bflags, 53));
        mu_assert(!BFLAGS_TEST(bflags, 125));

        BFLAGS_FLIP(bflags, 53);
        BFLAGS_FLIP(bflags, 125);

        mu_assert(BFLAGS_TEST(bflags, 53));
        mu_assert(BFLAGS_TEST(bflags, 125));
    }

    return NULL;
}

static char* bflags_and()
{
    {
        BFLAGS_DECLARE_ZERO(bflags_a, 64);
        BFLAGS_DECLARE_ZERO(bflags_b, 64);
        BFLAGS_DECLARE_ZERO(bflags_c, 64);

        BFLAGS_FILL(bflags_a, 64);

        BFLAGS_AND(bflags_c, bflags_a, bflags_b, 64);

        for (size_t i = 0; i != 64; ++i)
            mu_assert(!BFLAGS_TEST(bflags_c, i));
    }

    {
        BFLAGS_DECLARE_ZERO(bflags_a, 64);
        BFLAGS_DECLARE_ZERO(bflags_c, 64);

        BFLAGS_FILL(bflags_a, 64);

        BFLAGS_AND(bflags_c, bflags_a, bflags_a, 64);

        for (size_t i = 0; i != 64; ++i)
            mu_assert(BFLAGS_TEST(bflags_c, i));
    }

    {
        BFLAGS_DECLARE_ZERO(bflags_a, 64);
        BFLAGS_DECLARE_ZERO(bflags_b, 64);
        BFLAGS_DECLARE_ZERO(bflags_c, 64);

        for (size_t i = 0; i != 64; ++i)
            if (i % 2 == 0)
                BFLAGS_SET(bflags_a, i);

        for (size_t i = 0; i != 64; ++i)
            if (i % 2 != 0)
                BFLAGS_SET(bflags_b, i);

        BFLAGS_AND(bflags_c, bflags_a, bflags_b, 64);

        for (size_t i = 0; i != 64; ++i)
            mu_assert(!BFLAGS_TEST(bflags_c, i));
    }

    return NULL;
}

static char* bflags_or()
{
    {
        BFLAGS_DECLARE_ZERO(bflags_a, 64);
        BFLAGS_DECLARE_ZERO(bflags_b, 64);
        BFLAGS_DECLARE_ZERO(bflags_c, 64);

        BFLAGS_FILL(bflags_a, 64);

        BFLAGS_OR(bflags_c, bflags_a, bflags_b, 64);

        for (size_t i = 0; i != 64; ++i)
            mu_assert(BFLAGS_TEST(bflags_c, i));
    }

    {
        BFLAGS_DECLARE_ZERO(bflags_a, 64);
        BFLAGS_DECLARE_ZERO(bflags_c, 64);

        BFLAGS_FILL(bflags_a, 64);

        BFLAGS_OR(bflags_c, bflags_a, bflags_a, 64);

        for (size_t i = 0; i != 64; ++i)
            mu_assert(BFLAGS_TEST(bflags_c, i));
    }

    {
        BFLAGS_DECLARE_ZERO(bflags_a, 64);
        BFLAGS_DECLARE_ZERO(bflags_b, 64);
        BFLAGS_DECLARE_ZERO(bflags_c, 64);

        for (size_t i = 0; i != 64; ++i)
            if (i % 2 == 0)
                BFLAGS_SET(bflags_a, i);

        for (size_t i = 0; i != 64; ++i)
            if (i % 2 != 0)
                BFLAGS_SET(bflags_b, i);

        BFLAGS_OR(bflags_c, bflags_a, bflags_b, 64);

        for (size_t i = 0; i != 64; ++i)
            mu_assert(BFLAGS_TEST(bflags_c, i));
    }

    return NULL;
}

static char* bflags_xor()
{
    {
        BFLAGS_DECLARE_ZERO(bflags_a, 64);
        BFLAGS_DECLARE_ZERO(bflags_b, 64);
        BFLAGS_DECLARE_ZERO(bflags_c, 64);

        BFLAGS_FILL(bflags_a, 64);

        BFLAGS_XOR(bflags_c, bflags_a, bflags_b, 64);

        for (size_t i = 0; i != 64; ++i)
            mu_assert(BFLAGS_TEST(bflags_c, i));
    }

    {
        BFLAGS_DECLARE_ZERO(bflags_a, 64);
        BFLAGS_DECLARE_ZERO(bflags_c, 64);

        BFLAGS_FILL(bflags_a, 64);

        BFLAGS_XOR(bflags_c, bflags_a, bflags_a, 64);

        for (size_t i = 0; i != 64; ++i)
            mu_assert(!BFLAGS_TEST(bflags_c, i));
    }

    {
        BFLAGS_DECLARE_ZERO(bflags_a, 64);
        BFLAGS_DECLARE_ZERO(bflags_b, 64);
        BFLAGS_DECLARE_ZERO(bflags_c, 64);

        for (size_t i = 0; i != 64; ++i)
            if (i % 2 == 0)
                BFLAGS_SET(bflags_a, i);

        for (size_t i = 0; i != 64; ++i)
            if (i % 2 != 0)
                BFLAGS_SET(bflags_b, i);

        BFLAGS_XOR(bflags_c, bflags_a, bflags_b, 64);

        for (size_t i = 0; i != 64; ++i)
            mu_assert(BFLAGS_TEST(bflags_c, i));
    }

    return NULL;
}

static char* bflags_not()
{
    {
        BFLAGS_DECLARE_ZERO(bflags_a, 64);
        BFLAGS_DECLARE_ZERO(bflags_b, 64);

        for (size_t i = 0; i != 64; ++i)
            if (i % 2 == 0)
                BFLAGS_SET(bflags_a, i);

        BFLAGS_NOT(bflags_b, bflags_a, 64);

        for (size_t i = 0; i != 64; ++i)
            if (i % 2 == 0)
                mu_assert(!BFLAGS_TEST(bflags_b, i));
            else
                mu_assert(BFLAGS_TEST(bflags_b, i));
    }

    {
        BFLAGS_DECLARE_ZERO(bflags_a, 64);

        for (size_t i = 0; i != 64; ++i)
            if (i % 2 == 0)
                BFLAGS_SET(bflags_a, i);

        BFLAGS_NOT(bflags_a, bflags_a, 64);

        for (size_t i = 0; i != 64; ++i)
            if (i % 2 == 0)
                mu_assert(!BFLAGS_TEST(bflags_a, i));
            else
                mu_assert(BFLAGS_TEST(bflags_a, i));
    }

    return NULL;
}

static char* run_all_tests()
{
    mu_run_test(bflags_test_declare);
    mu_run_test(bflags_test_declare_zero);

    mu_run_test(bflags_zero);
    mu_run_test(bflags_fill);

    mu_run_test(bflags_copy);
    mu_run_test(bflags_eq);

    mu_run_test(bflags_set);
    mu_run_test(bflags_clear);
    mu_run_test(bflags_flip);

    mu_run_test(bflags_test);

    mu_run_test(bflags_and);
    mu_run_test(bflags_or);
    mu_run_test(bflags_xor);
    mu_run_test(bflags_not);

    return NULL;
}

int tests_run = 0;

int main(int UNUSED(argc), char** UNUSED(argv))
{
    char* result = run_all_tests();

    if (result)
        printf("TEST FAILED: %s\n", result);
    else
        printf("PASS\n");

    printf("\ndone! %d tests run\n", tests_run);

    return (result != NULL);
}

An online version can be found here.

Thanks!

\$\endgroup\$
5
\$\begingroup\$

Small portability bug: if we're using false in the macros, then bflags.h should include <stdbool.h>. Or use 0 instead of false.

Definite portability bug: we shift a (signed) int before promoting:

#define BFLAGS_WORD_MASK(bit) (1 << ((bit) % BFLAGS_WORD_BIT_SIZE))

I think we need the 1 there to be of the appropriate type first:

#define BFLAGS_WORD_MASK(bit) ((BFLAGS_WORD_T)1 << ((bit) % BFLAGS_WORD_BIT_SIZE))

Similarly, this test:

    BFLAGS_SET(bflags, 31);
    mu_assert(bflags[0] == ((1 << 0) | (1 << 5) | (1 << 31)));

needs

    BFLAGS_SET(bflags, 31);
    mu_assert(bflags[0] == ((1 << 0) | (1 << 5) | (1ul << 31)));

There's quite a few more that I picked up with gcc -Wall -Wextra and are easily fixed.

Also, let's be good about const char* - all the test results should be pointer to const string, and it doesn't hurt to fix that.

The final compilation warning I see is from char** UNUSED(argv) - easily fixed by changing it to UNUSED(char** argv). But since we're ignoring both arguments, we can use the other legal signature: int main(void).


Style-wise, we could use a typedef instead of #define for BFLAGS_WORD_T. Instead of the optional fixed-width type, we should probably use uint_fast32_t, as we don't really mind if uint32_t doesn't exist (on some exotic or ancient architecture). In fact, we might prefer plain unsigned int, as that's supposed to be the hardware's "natural" (most efficient) integer size. Everything in the code appears to adapt suitably, except for some of the tests that hard-code the 32-bit assumption.

BTW, if using a typedef, remember that names ending in _t are reserved for the library.

I do have reservations about the use of macros (rather than functions) when users may pass plain int rather than unsigned types as arguments - for example, we get unnecessary promotion in BFLAGS_ARRAY_SIZE() if we add a signed bits to unsigned BFLAGS_WORD_BIT_SIZE. Perhaps these should be simple (inlinable) functions to give us strong typing?


We can reduce repetition here:

#define BFLAGS_DECLARE(bflags, bits) \
    BFLAGS_WORD_T bflags[BFLAGS_ARRAY_SIZE(bits)]

#define BFLAGS_DECLARE_ZERO(bflags, bits) \
    BFLAGS_WORD_T bflags[BFLAGS_ARRAY_SIZE(bits)] = { 0 }

like this:

#define BFLAGS_DECLARE_ZERO(bflags, bits) \
    BFLAGS_DECLARE(bflags, bits) = { 0 }
\$\endgroup\$
5
\$\begingroup\$

Most severe issue:

  • Never invent secret macro languages! This is about the single-worst thing a C programmer can ever do, all categories.

    You are perfectly free to assume that any C programmer will understand what 1<<bit means. You cannot assume that they will understand what BFLAGS_WORD_MASK means. It's nothing but obfuscation of what would otherwise have been clear code.

    Therefore I would strongly recommend you to drop this whole idea. Many before you have tried this exact thing and it has always ended badly.

Big picture issues:

  • 0x00000001L creates a signed integer constant of type long. You should never use signed type for bitwise operations. Change this to 0x00000001UL.

    The danger is that signed types may invoke undefined behavior bugs when sign bits are accidentally set. And there's almost never a reason to use bitwise operators on signed types.

    In addition, it is dangerous to type out all zeroes of hex constants, because they actually don't have any significance and don't make the number 32 bits as expected. You can get real nasty bugs this way. For example, lets say we have a 16 bit system and type this:

    #define BLIP 0x00007FFF  // this is type signed int 16 bit
    #define BLOP 0x00008000  // this is type unsigned int 16 bit
    #define BLUP 0x00010000  // this is type long 32 bit
    

    You can get extremely subtle bugs this way. Been there, very hard to track down. Suffix all such hex constants with UL.

  • For the same reason, enums should never be used for bitwise operations, because an enumeration constant is always int and the compiler cannot change the type without violating the C standard. Therefore you should not use enum for this.

    And you should not store long or unsigned long constants in an enum, because they will get converted to int which is wrong and bad.

The above means your whole code must be rewritten from scratch.

Other problems:

  • Avoid bit-shifting 1 since it is of type int and can give the same problems as mentioned above. Use 1u instead.

  • Don't declare functions in C as static char* bflags_or() but use static char* bflags_or (void). The empty parenthesis () means "accept any type", which is not what you want. This style has been flagged as obsolete in the C standard and can be removed at any point.

  • There is no reason to use non-standard int UNUSED(argc). Instead write this:

    int main (int argc, char** argv)
    {
      (void)argc;
      (void)argv;
    

    This is 100% portable and standard, achieving the same thing as non-standard UNUSED.

  • Don't use non-standard #pragma once, use classic header guards #ifndef SOMETHING_H #define SOMETHING_H ... #endif. 100% portable and standard. Avoid pragmas in general.

Style remarks:

  • It is good practice to always use compound statements {} after if and loops, regardless of how many lines that the body contains. This reduces the amount of indention and maintenance related bugs. And as a bonus you don't have to use do-while(0) macro tricks any longer, if you don't allow selection/loop statements without {}.
\$\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.