Populate a color table in XCB

Question

This function has 16 repeated calls to the same function, passing in the same handles to each:

// Create all 16 colors
void fill_color_gcs(xcb_connection_t *c, xcb_window_t w, xcb_screen_t *s,
                xcb_gcontext_t *col) {
        col[0] = color_gc(c, w, s, 0, 0, 0);
        col[1] = color_gc(c, w, s, 1, 1, 1);
        col[2] = color_gc(c, w, s, 1, 0, 0);
        col[3] = color_gc(c, w, s, 0, 1, 1);
        col[4] = color_gc(c, w, s, 1, 0, 1);
        col[5] = color_gc(c, w, s, 0, 1, 0);
        col[6] = color_gc(c, w, s, 0, 0, 1);
        col[7] = color_gc(c, w, s, 1, 1, 0);
        col[8] = color_gc(c, w, s, 1, 0.5, 0);
        col[9] = color_gc(c, w, s, 0.5, 0.25, 0.25);
        col[10] = color_gc(c, w, s, 1, 0.5, 0.5);
        col[11] = color_gc(c, w, s, 0.25, 0.25, 0.25);
        col[12] = color_gc(c, w, s, 0.5, 0.5, 0.5);
        col[13] = color_gc(c, w, s, 0.5, 1, 0.5);
        col[14] = color_gc(c, w, s, 0.5, 0.5, 1);
        col[15] = color_gc(c, w, s, 0.75, 0.75, 0.75);
}

Alternatively, I can "let the preprocessor copy-paste for us" instead, like this:

// Create all 16 colors
#define COLOR_GC(I, R, G, B) col[I] = color_gc(c, w, s, R, G, B)
void fill_color_gcs(xcb_connection_t *c, xcb_window_t w, xcb_screen_t *s,
                xcb_gcontext_t *col) {
        COLOR_GC(0, 0, 0, 0);
        COLOR_GC(1, 1, 1, 1);
        COLOR_GC(2, 1, 0, 0);
        COLOR_GC(3, 0, 1, 1);
        COLOR_GC(4, 1, 0, 1);
        COLOR_GC(5, 0, 1, 0);
        COLOR_GC(6, 0, 0, 1);
        COLOR_GC(7, 1, 1, 0);
        COLOR_GC(8, 1, 0.5, 0);
        COLOR_GC(9, 0.5, 0.25, 0.25);
        COLOR_GC(10, 1, 0.5, 0.5);
        COLOR_GC(11, 0.25, 0.25, 0.25);
        COLOR_GC(12, 0.5, 0.5, 0.5);
        COLOR_GC(13, 0.5, 1, 0.5);
        COLOR_GC(14, 0.5, 0.5, 1);
        COLOR_GC(15, 0.75, 0.75, 0.75);
}

It would be more compact, and theoretically I would only need to change the macro if all 16 lines need to change at the same time (let's say, color_gc changes). However, I have reverted back to using "explicit" copy-pasting as the macros actually felt harder to read after a while as it hides information on how it actually works.

Which way is better practice? I don't think an additional function would work here since the handles still need to be passed in regardless.

Answer 1

For both functions, consider lining up the numbers so the tabular nature is easier to read:

void fill_color_gcs(xcb_connection_t *c, xcb_window_t w,
                    xcb_screen_t *s, xcb_gcontext_t *col)
{
    col[0]  = color_gc(c, w, s,  0,    0,    0   );
    col[1]  = color_gc(c, w, s,  1,    1,    1   );
    col[2]  = color_gc(c, w, s,  1,    0,    0   );
    col[3]  = color_gc(c, w, s,  0,    1,    1   );
    col[4]  = color_gc(c, w, s,  1,    0,    1   );
    col[5]  = color_gc(c, w, s,  0,    1,    0   );
    col[6]  = color_gc(c, w, s,  0,    0,    1   );
    col[7]  = color_gc(c, w, s,  1,    1,    0   );
    col[8]  = color_gc(c, w, s,  1,    0.5,  0   );
    col[9]  = color_gc(c, w, s,  0.5,  0.25, 0.25);
    col[10] = color_gc(c, w, s,  1,    0.5,  0.5 );
    col[11] = color_gc(c, w, s,  0.25, 0.25, 0.25);
    col[12] = color_gc(c, w, s,  0.5,  0.5,  0.5 );
    col[13] = color_gc(c, w, s,  0.5,  1,    0.5 );
    col[14] = color_gc(c, w, s,  0.5,  0.5,  1   );
    col[15] = color_gc(c, w, s,  0.75, 0.75, 0.75);
}

---

In the second function, #undef the macro as soon as you have finished expanding it. That reduces macro namespace pollution, and prevents a warning when other code wishes to define a macro with the same name.

---

Consider using an initialiser table and loop:

void fill_color_gcs(xcb_connection_t *c, xcb_window_t w,
                    xcb_screen_t *s, xcb_gcontext_t *col)
{
    static const double colors[16][3] = {
        {0,    0,    0   },
        {1,    1,    1   },
        {1,    0,    0   },
        {0,    1,    1   },
        {1,    0,    1   },
        {0,    1,    0   },
        {0,    0,    1   },
        {1,    1,    0   },
        {1,    0.5,  0   },
        {0.5,  0.25, 0.25},
        {1,    0.5,  0.5 },
        {0.25, 0.25, 0.25},
        {0.5,  0.5,  0.5 },
        {0.5,  1,    0.5 },
        {0.5,  0.5,  1   },
        {0.75, 0.75, 0.75}
    };
    for (unsigned i = 0;  i < sizeof rgb / sizeof rgb[0];  ++i) {
        const double rgb[3] = colors[i];
        col[i] = color_gc(c, w, s, rgb[0], rgb[1], rgb[2]);
    }
}

Answer 2

For me it looks like you create some kind of color table here. In the first step I would make the function more flexible by giving the colors as parameter

void fill_color_gcs(xcb_connection_t *c, xcb_window_t w, xcb_screen_t *s,
            xcb_gcontext_t *col, float *colorTable)

and then just loop through the table

for (int i = 0; i < 16; i++)
    col[i] = color_gc(c, w, s, colorTable[i][0], colorTable[i][1], colorTable[i][2]);

The colorTable should be created before calling this function.

The advantage is, that you can now create the colorTable for example from a config file instead of having it hardcoded. This makes your software much more flexible. You don't need to recompile your code if you want to change a color.

Answer 3

I think the way you applied the macro in this case is spot on. My only concern is the mixture of integer and floating-point values. I would recommend replacing it with something like this that also looks a bit neater:

void fill_color_gcs(xcb_connection_t * c, xcb_window_t w, xcb_screen_t * s, 
    xcb_gcontext_t * col) {
    COLOR_GC(0, 0.00, 0.00, 0.00);
    COLOR_GC(1, 1.00, 1.00, 1.00);
    COLOR_GC(2, 1.00, 0.00, 0.00);
    COLOR_GC(3, 0.00, 1.00, 1.00);
    COLOR_GC(4, 1.00, 0.00, 1.00);
    COLOR_GC(5, 0.00, 1.00, 0.00);
    COLOR_GC(6, 0.00, 0.00, 1.00);
    COLOR_GC(7, 1.00, 1.00, 0.00);
    COLOR_GC(8, 1.00, 0.50, 0.00);
    COLOR_GC(9, 0.50, 0.25, 0.25);
    COLOR_GC(10, 1.00, 0.50, 0.50);
    COLOR_GC(11, 0.25, 0.25, 0.25);
    COLOR_GC(12, 0.50, 0.50, 0.50);
    COLOR_GC(13, 0.50, 1.00, 0.50);
    COLOR_GC(14, 0.50, 0.50, 1.00);
    COLOR_GC(15, 0.75, 0.75, 0.75);
}

I agree with Claus Bönnhoff suggestion of using a lookup table, but this will create unnecessary code if the colors will never change. I’m beginning to understand why your question was downvoted.