Using bitwise operators with (unsigned long) instead of bitfields

Question

Here is my solution to Programming Exercise 15-7 from Stephen Prata's C Primer Plus, 6th Edition. This problem asks the reader to:

15-7
Write a program with the same behavior as described in exercise 6, but use an unsigned long variable to hold the font information and use the bitwise operators instead of bit members to manage the information.

The previous exercise, referenced in the question, was:

15-6
Design a bit-field structure that holds the following information:

• Font ID: A number in the range 0--255

• Font Size: A number in the range 0--127

• Alignment: A number in the range 0--2 representing the choices Left, Center, and Right

• Bold: Off (0) or on (1)

• Italic: Off (0) or on (1)

• Underline: Off (0) or on (1)

Use this structure in a program that displays the font parameters and uses a looped menu to let the user change parameters....
...
[sample interaction here]
...
The program should use the & operator and suitable masks to ensure that the ID and size entries are converted to the specified range.

A sample run is provided that illustrates the desired formatting for a few interactions. I did not think that this was terribly pertinent to my question, but here is a Pastebin link to a text file containing the whole of both questions for anyone who is interested. There is an assignment at the beginning of main() that sets font_params to match the starting point of the sample interaction.

This code works and seems to handle bad inputs acceptably.

I am trying to better understand how to use bitwise operations. I am interested in any comments about the style or robustness of my code, especially as they pertain to its bitwise aspect or how I might have better used bitwise operations and better structured the font parameter bit assignments. I am not certain that I understood the last sentence of the question (15-6) about using "the & operator and suitable masks to ensure that the ID and size entries are converted to the specified range." I do use both an & operator and a mask, but in my code this is used for a loop test; there is no conversion of an input value.

Any comments are welcome, but I would also be particularly interested in comments about:

• The use of strtoul() in the get_selection() function to convert an input string to a number
• The use of strcspn() in the get_selection() function to convert an input string to an ALIGNMENT number
• The structure of the user input loop in get_selection()

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

/***********************************************************************
 *                    Font Parameter Bit Assignments                   *
 *                                                                     *
 * Property:        ID          SZ         ALN   BLD  ITL  UND         *
 * Offsets:                12         5     3    2    1    0           *
 * Bits:            11111111    1111111    11    1    1    1           *
 * Hex Value:       0xFF000     0xFE0      0x18  0x4  0x2  0x1         *
 *                                                                     *
 ***********************************************************************/

#define UNDERLINE   0x1
#define ITALIC      0x2
#define BOLDFACE    0x4
#define ALIGNMENT   0x18
#define FONT_SZ     0x0FE0
#define FONT_ID     0xFF000
#define UND_OFFSET  0
#define ITL_OFFSET  1
#define BLD_OFFSET  2
#define ALN_OFFSET  3
#define SZ_OFFSET   5
#define ID_OFFSET   12

#define BUFSIZE     5

char * s_gets(char *st, int n);
void show_params(const unsigned long params);
void show_menu(void);
int get_selection(unsigned long *params);

int main(void)
{
    unsigned long font_params = 0;

    /* Assign initial values to font_params: zero initialization  */
    /* gives LEFT alignment with underline, italics, and bold off */
    font_params |= (1 << ID_OFFSET | 12 << SZ_OFFSET);

    do {
        show_params(font_params);
        show_menu();
    } while (get_selection(&font_params));

    puts("Bye!");

    return 0;
}

char * s_gets(char *st, int n)
{
    char *ret_val;
    char *find;

    ret_val = fgets(st, n, stdin);
    if (ret_val) {
        find = strchr(st, '\n');
        if (find)
            *find = '\0';
        else
            while (getchar() != '\n')
                continue;
    }

    return ret_val;
}

void show_params(const unsigned long params)
{
    static char algn[3][10] = {"left   ", "center   ", "right   "};
    static char state[2][4] = {"off", " on"};

    putchar('\n');
    printf("%4s %4s %9s %3s %3s %3s\n",
           "ID", "SIZE", "ALIGNMENT", " B ", " I ", " U ");
    printf("%4lu %4lu %9s %3s %3s %3s\n",
           (params & FONT_ID) >> ID_OFFSET,
           (params & FONT_SZ) >> SZ_OFFSET,
           algn[(params & ALIGNMENT) >> ALN_OFFSET],
           state[(params & BOLDFACE) >> BLD_OFFSET],
           state[(params & ITALIC) >> ITL_OFFSET],
           state[(params & UNDERLINE) >> UND_OFFSET]);
    putchar('\n');
}

void show_menu(void)
{
    puts("f)change font    s)change size    a)change alignment");
    puts("b)toggle bold    i)toggle italic  u)toggle underline");
    puts("q)quit");
}

int get_selection(unsigned long *params)
{
    char buf[BUFSIZE];
    char *tailptr;                  // for use with strtoul()
    char options[] = "lcrLCR";      // a string of available options,
    size_t input;                    // in both lower and upper cases

    for (;;) {
        s_gets(buf, 2);

        switch (buf[0]) {
        case 'f':
        case 'F':
            do {
                printf("Enter font ID (0-255): ");
                s_gets(buf, BUFSIZE);
                input = strtoul(buf, &tailptr, 10);
            } while (*tailptr || (input & ~0xFF));
            *params &= ~FONT_ID;    // clear before setting bits
            *params |= input << ID_OFFSET;
            break;

        case 's':
        case 'S':
            do {
                printf("Enter font size (0-127): ");
                s_gets(buf, BUFSIZE);
                input = strtoul(buf, &tailptr, 10);
            } while (*tailptr || (input & ~0x7F));
            *params &= ~FONT_SZ;    // clear before setting bits
            *params |= input << SZ_OFFSET;
            break;

        case 'a':
        case 'A':
            do {
                puts("Select alignment:");
                puts("l)left   c)center   r)right");
                s_gets(buf, 2);
                input = strcspn(options, buf);
            } while (input > strlen(options));
            *params &= ~ALIGNMENT;  // clear before setting bits
            *params |= (input % (strlen(options) / 2) << ALN_OFFSET);
            break;

        case 'b':
        case 'B':
            *params ^= BOLDFACE;
            break;

        case 'i':
        case 'I':
            *params ^= ITALIC;
            break;

        case 'u':
        case 'U':
            *params ^= UNDERLINE;
            break;

        case 'q':
        case 'Q':
            return 0;               // signal to quit
            break;

        default:
            puts("Please enter a valid menu choice.");
            show_menu();
            continue;
        }
        break;
    }

    return 1;                       // successful selection
}

Answer 1

1. An int may be narrower than an unsigned long. Avoid unexpected behavior by using the correct type.

   unsigned long font_params = 0;
   // font_params |= (1 << ID_OFFSET | 12 << SZ_OFFSET);
   font_params |= (1ul << ID_OFFSET | 12ul << SZ_OFFSET);
   

2. Avoid an infinite loop

   // while (getchar() != '\n') continue;
   int ch;
   while ((ch = getchar()) != '\n' && ch != EOF) continue;
   

3. Consider specifying widths - easier to update/maintain.

   // printf("%4s %4s %9s %3s %3s %3s\n", "ID", "SIZE", "ALIGNMENT", " B ", " I ", " U ");
   // printf("%4lu %4lu %9s %3s %3s %3s\n", 
   //     (params & FONT_ID) >> ID_OFFSET, 
   //     (params & FONT_SZ) >> ID_OFFSET, ...
   
   int w[6] = { 4,4,9,3,3,3 };
   printf("%*s %*s %*s ...", w[0], "ID", w[1], "SIZE", w[2], "ALIGNMENT", ...
   printf("%*lu %*lu %*s ...\n", 
       w[0], (params & FONT_ID) >> ID_OFFSET, 
       w[1], (params & FONT_SZ) >> ID_OFFSET, ...
   

4. Prefer to use matching types. Use unsigned long for strtoul() and size_t for strcspn().

   // size_t input;
   unsigned long input;
   
   input = strtoul(buf, &tailptr, 10);
   
   size_t size;
   
   // input = strcspn(options, buf);
   size = strcspn(options, buf);
   

5. Watch out for assuming ~ applies to the width of the target - it is applied to the constant. What if unsigned long was 64-bit, unsigned was 32-bit and FONT_SZ was 0xFE000000. ~FONT_SZ is an unsigned with the value of 0x01FFFFFF rather than 0xFFFFFFFF01FFFFFF, thus masking off the top bits. Same problem with ~0xFF anf ~0x7F. Better to use ~0xFFul, etc.

   unsigned long *params;
   // *params &= ~FONT_SZ; 
   *params &= ~(1ul*FONT_SZ); 
   // or 
   *params |= FONT_SZ; 
   *params ^= FONT_SZ; 
   // or 
   #define FONT_SZ     0x0FE0ul
   

6. Unclear why char buf[BUFSIZE]; is so small (5). I'd expect a buffer width enough to handle the widest unsigned long value and then twice that. No reason to be stingy in buffer size in user input of a single number.

   #define ULONG_STR10_LEN (sizeof(unsigned long)*CHAR_BIT/3 + 1) 
   #define BUF_SIZE (ULONG_STR10_LEN*2 + 1 /* \n */ + 1 /* \0 */) 
   char buf[BUF_SIZE];`