7
\$\begingroup\$

In answering How to do file I/O with bool in C?, I made a helper function that used fgetc() calls to substitute the missing fscanf() bool * specifier.

Also included is a test harness.

Review goals:

  • Primary: Review fscanf_bool() for how well it achieved the above goal as well as any other concerns.

  • Secondary: Review the test harness for completeness: How thoroughly did it exercise fscanf_bool()? Suggested additional tests?


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

/*
 * Read a 'bool' as if scanf() had a 'bool *' specifier.
 *
 * Read stream until:
 * * consume and ignore leading white-space.
 * * a complete case-less match to: "0", "1", "f", "t", "false", "true",
 * * end-of-file, or input error.
 * Non-matching characters are pushed back.
 *
 * On success, assign *b as true or false (1 or 0).
 *
 * Return 1 on success.
 * EOF on input error or immediate end-of-file.
 * 0 otherwise.
 */
int fscanf_bool(FILE *inf, bool *b) {
  int buf[5];
  int first_ch;

  // Ignore leading whitespaces.
  do {
    first_ch = buf[0] = fgetc(inf);
  } while (isspace(first_ch));

  // Test if the first character is enough.
  if (first_ch == '0' || first_ch == '1') {
    *b = (first_ch == '1');
    return 1;
  }
  if (first_ch == EOF) {
    return EOF;
  }
  first_ch = tolower(first_ch);
  if (first_ch != 'f' && first_ch != 't') {
    ungetc(buf[0], inf);
    return 0;
  }

  // Attempt to match "false" or "true".
  bool value = (first_ch == 't');
  static const char *ft[2] = {"false", "true"};
  const char *match = ft[value];
  for (unsigned i = 1; match[i]; i++) {
    buf[i] = fgetc(inf);
    int next_ch = tolower(buf[i]);
    if ((next_ch == EOF) && !feof(inf)) { // Input error
      return EOF;
    }
    if (next_ch != match[i]) {
      do {
        // Attempt to unget all "next" characters.
        // The first attempt should work if not EOF.
        // Subsequent ones may work, may fail.
        ungetc(buf[i--], inf);
      } while (i > 0);
      break;
    }
  }
  *b = value;
  return 1;
}

/*
 * Test code
 */

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

int test_fscanf_bool(const char *mode, bool initial, bool final, int rv,
    char next_ch, const char *stream_data) {
  int error = 0;
  bool b = initial;

  char *filename = "tmp.txt";
  FILE *outf = fopen(filename, "w");
  assert(outf);
  fputs(stream_data, outf);
  fclose(outf);

  FILE *inf = fopen(filename, mode);
  assert(inf);
  int retval = fscanf_bool(inf, &b);
  if (b != final) {
    printf("<%s> b:%d != expected b:%d\n", stream_data, b, final);
    error++;
  }
  if (retval != rv) {
    printf("<%s> retval:%d != expected retval:%d\n", stream_data, retval, rv);
    error++;
  }
  int ch = fgetc(inf);
  if (ch != next_ch) {
    printf("<%s> ch:%d != expected ch:%d\n", stream_data, ch, next_ch);
    error++;
  }
  fclose(inf);
  return error;
}

typedef struct {
  const char *mode;
  bool initial;
  bool final;
  int rv;
  char next_ch;
  const char *stream_data;
} test_data;

int tests_fscanf_bool(void) {
  test_data td[] = { //
      {"r", false, false, 1, 'x', "0x"}, // Happy path
          {"r", false, true, 1, 'x', "1x"}, //
          {"r", false, false, 1, 'x', "fx"}, //
          {"r", false, true, 1, 'x', "tx"}, //
          {"r", false, false, 1, 'x', "Fx"}, //
          {"r", false, true, 1, 'x', "Tx"}, //
          {"r", false, false, 1, 'x', "Falsex"}, //
          {"r", false, true, 1, 'x', "Truex"}, //
          {"r", false, false, 1, 'x', " \t\n\r\v0x"}, //
          {"r", false, false, 1, 'x', " 0x"}, //
          {"r", false, false, 0, 'X', "X"}, // Wrong upper char
          {"r", true, true, 0, 'x', "x"}, // Wrong lower char
          {"r", false, false, EOF, EOF, " \t\n\r\v"}, // Spaces
          {"r", false, false, EOF, EOF, ""}, // Empty
          {"w", false, false, EOF, EOF, "0x"}, // Input error
          // Below rely on more than 1 ungetc()
          {"r", false, false, 1, 'a', "Falsx"}, // Happy path
          {"r", false, true, 1, 'R', "TRux"}, //
      };
  size_t n = sizeof td / sizeof td[0];
  int error = 0;
  for (size_t i = 0; i < n; i++) {
    int e = test_fscanf_bool(td[i].mode, td[i].initial, td[i].final, td[i].rv,
        td[i].next_ch, td[i].stream_data);
    error += e;
    if (e)
      break;  // Comment out to run all tests.
  }
  printf("Error count %d\n", error);
  return error;
}

int main(void) {
  return tests_fscanf_bool() ? EXIT_FAILURE : EXIT_SUCCESS;
}

Output

Error count 0
\$\endgroup\$
3
  • \$\begingroup\$ Pedantic note: && !feof(inf) used instead of && ferror(inf) to distinguish case when error flag is already set prior to the function call and end-of-file now occurs is not the same as input error having just occurred. \$\endgroup\$ Nov 13, 2023 at 6:18
  • \$\begingroup\$ Is it undefined behavior to call it like so: fscanf_bool(NULL, NULL);? \$\endgroup\$
    – Harith
    Nov 13, 2023 at 7:57
  • 2
    \$\begingroup\$ @Harith Yes. It is UB, just like fscanf(NULL, "%d", valid_ptr) and fscanf(valid_ptr, "%d", NULL) are. \$\endgroup\$ Nov 13, 2023 at 11:36

1 Answer 1

8
\$\begingroup\$

Ooh, unit tests! You know how to make me happy!

Let's look at the tests first, as that's valuable documentation of the expected use cases.

A purist might argue that these are not unit tests because they depend on an external resource (a filesystem where we can write tmp.txt), but that's understandable in C, which has no easy way to create any other FILE*.

And there's a minor mistake:

287905.c: In function ‘test_fscanf_bool’:
287905.c:81:20: warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
   81 |   char *filename = "tmp.txt";
      |                    ^~~~~~~~~

We can do better, though, by using tmpfile() to create the file somewhere more appropriate:

  FILE *f = tmpfile();
  if (!f) {
      perror("tmpfile");
      abort();
  }
  if (fputs(stream_data, f) == EOF || fflush(f)) {
      perror("fputs");
      abort();
  }
  f = freopen(NULL, mode, f);
  if (!f) {
      perror("reopen");
      abort();
  }

We have a problem with the test data structure:

typedef struct {
  const char *mode;
  bool initial;
  bool final;
  int rv;
  char next_ch;
  const char *stream_data;
} test_data;

If we want to use EOF in next_ch, that member really needs to be an int so that we can distinguish from a valid char that compares equal to EOF. We'll also need to change the corresponding argument of test_fscanf_bool().

Actually, the arguments to test_fscanf_bool() exactly copy the test_data struct, so it may be better to simply pass a pointer to the entry in the test array.


None of the test cases demonstrate a true initial becoming false - we should ensure that we're not just skipping the assignment.

We also have no test for e.g. tr - demonstrate that we correctly back up from end-of-file during the lookahead.


if (e)
  break;  // Comment out to run all tests.

I recommend running all the tests always. Exiting on the first failure is frustrating when debugging. When this early exit is enabled, the "error count" message is misleading.


Let's move on to the implementation now.

I'd change the if chain to a switch:

  switch (first_ch) {
  case EOF:
      return EOF;
  case '0':
      *b = false;
      return 1;
  case '1':
      *b = true;
      return 1;
  case 'f': case 'F':
      *b = false;
      break;
  case 't': case 'T':
      *b = true;
      break;
  default:
      /* not a bool string */
      ungetc(first_ch, inf);
      return 0;
  }

Then can move the declaration of buf down to just before the loop where it's used.

I would declare the static variable ft const:

  static const char *const ft[2] = {"false", "true"};

I'm a bit uncomfortable ignoring the return value of fungetc() for the non-EOF values. That said, it's hard to know what we should do when the target's push-back buffer is too small for our purposes; maybe ignoring it is correct.

We don't really need next_ch (or for that matter, first_ch). I don't think it's any less clear to use the buf[] entries.


Modified code

Some of my changes here are personal preferences, but pick and choose what you like:

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

/*
 * Read a 'bool' as if scanf() had a 'bool *' specifier.
 *
 * Read stream until:
 * * consume and ignore leading white-space.
 * * a complete case-less match to: "0", "1", "f", "t", "false", "true",
 * * end-of-file, or input error.
 * Non-matching characters are pushed back.
 *
 * On success, assign *b as true or false (1 or 0).
 *
 * Return 1 on success.
 * EOF on input error or immediate end-of-file.
 * 0 otherwise.
 */
int fscanf_bool(FILE *inf, bool *b)
{
    int buf[5];

    // Ignore leading whitespace.
    do {
        buf[0] = fgetc(inf);
    } while (isspace(buf[0]));

    // Test the first character.
    switch (buf[0]) {
    case EOF:
        return EOF;
    case '0':
        *b = false;
        return 1;
    case '1':
        *b = true;
        return 1;
    case 'f': case 'F':
        *b = false;
        break;
    case 't': case 'T':
        *b = true;
        break;
    default:
        /* not a bool string */
        ungetc(buf[0], inf);
        return 0;
    }

    // If we got here, we saw a 't' or 'f' (modulo case).
    // Advance past full word "false" or "true".
    static const char *const ft[2] = {"false", "true"};
    const char *match = ft[*b];
    for (unsigned i = 1;  match[i];  i++) {
        buf[i] = fgetc(inf);
        if ((buf[i] == EOF) && !feof(inf)) {
            // Input error
            return EOF;
        }
        if (tolower(buf[i]) != match[i]) {
            do {
                // Attempt to unget all "next" characters.
                // The first attempt should work, ignoring EOF.
                // Subsequent ones may work, but we ignore failures.
                ungetc(buf[i--], inf);
            } while (i > 0);
            break;
        }
    }
    return 1;
}
/*
 * Test code
 */

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

typedef struct {
    char mode[2];
    bool initial;
    bool final;
    int rv;
    int next_ch;
    char stream_data[10];
} test_data;

int test_fscanf_bool(const test_data *data)
{
    FILE *f = tmpfile();
    if (!f) {
        perror("tmpfile");
        abort();
    }
    if (fputs(data->stream_data, f) == EOF || fflush(f)) {
        perror("fputs");
        abort();
    }
    f = freopen(NULL, data->mode, f);
    if (!f) {
        perror("reopen");
        abort();
    }

    int error = 0;
    bool b = data->initial;
    int retval = fscanf_bool(f, &b);
    if (b != data->final) {
        printf("<%s> b:%d != expected b:%d\n",
               data->stream_data, b, data->final);
        ++error;
    }
    if (retval != data->rv) {
        printf("<%s> retval:%d != expected retval:%d\n",
               data->stream_data, retval, data->rv);
        ++error;
    }
    int ch = fgetc(f);
    if (ch != data->next_ch) {
        printf("<%s> ch:%d != expected ch:%d\n",
               data->stream_data, ch, data->next_ch);
        ++error;
    }
    fclose(f);
    return error;
}

int tests_fscanf_bool(void)
{
    static const test_data td[] = {
        {"r", false, false, 1, 'x', "0x"}, // Happy path
        {"r", false, true, 1, 'x', "1x"},
        {"r", false, false, 1, 'x', "fx"},
        {"r", true, false, 1, 'x', "fx"},  // yes, we overwrite
        {"r", false, true, 1, 'x', "tx"},
        {"r", false, false, 1, 'x', "Fx"},
        {"r", false, true, 1, 'x', "Tx"},
        {"r", false, false, 1, 'x', "Falsex"},
        {"r", false, true, 1, 'x', "Truex"},
        {"r", false, false, 1, 'x', " \t\n\r\v0x"},
        {"r", false, false, 1, 'x', " 0x"},
        {"r", false, false, 0, 'X', "X"}, // Wrong upper char
        {"r", true, true, 0, 'x', "x"}, // Wrong lower char
        {"r", false, true, 1, EOF, "T"}, // EOF during lookahead
        {"r", false, true, 1, 'R', "TR"}, // EOF during lookahead
        {"r", false, false, EOF, EOF, " \t\n\r\v"}, // Spaces
        {"r", false, false, EOF, EOF, ""}, // Empty
        {"w", false, false, EOF, EOF, "0x"}, // Input error (write-only stream)
        // Below rely on more than 1 ungetc()
        {"r", false, false, 1, 'a', "Falsx"}, // Happy path
        {"r", false, true, 1, 'R', "TRux"},
    };
    size_t n = sizeof td / sizeof td[0];
    int error = 0;
    for (size_t i = 0;  i < n;  ++i) {
        error += test_fscanf_bool(td + i);
        //if (error) { break; }  // Uncomment to quit on first failure.
    }
    printf("Error count %d\n", error);
    return error;
}

int main(void)
{
    return tests_fscanf_bool() ? EXIT_FAILURE : EXIT_SUCCESS;
}
```
\$\endgroup\$
12
  • 1
    \$\begingroup\$ Re; "warning: initialization discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]" --> char *filename = "tmp.txt"; is a char * initializing a char *. const would be a good idea, yet the warning looks like a C++ one. \$\endgroup\$ Nov 13, 2023 at 11:44
  • \$\begingroup\$ Yes, member char next_ch; should be int next_ch;. \$\endgroup\$ Nov 13, 2023 at 11:52
  • \$\begingroup\$ Yes, "None of the test cases demonstrate a true initial becoming false" is a good catch. \$\endgroup\$ Nov 13, 2023 at 12:11
  • \$\begingroup\$ Yes, 2nd const good for static const char *const ft[2]. \$\endgroup\$ Nov 13, 2023 at 12:13
  • 2
    \$\begingroup\$ Same problem exist today with fscanf(inf, "%f%c", &f, &ch) and input like "0xy". Since that "0x" could begin a "0x123.45p67", yet does not, y gets pushed back and maybe x. So what does ch get? \$\endgroup\$ Nov 13, 2023 at 12:30

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.