Any comment, support or objection on this one? I'm aware that this diff is hard to review, but doing it in several steps won't simplify the main step all that much, I think.
The regression tests cover this part of the code quite well, I think. ----- Forwarded message from Theo Buehler <[email protected]> ----- Date: Sat, 6 Aug 2016 17:51:10 +0200 From: Theo Buehler <[email protected]> To: [email protected] Subject: jot: remove crazy switch inside while loop jot.c has a loop containing a 16-way switch that fiddles with hardcoded values: while (mask) /* 4 bit mask has 1's where last 4 args were given */ switch (mask) { /* fill in the 0's by default or computation */ case 001: reps = REPS_DEF; mask = 011; It is very hard to follow what happens in there and most cases can't be touched without the risk of affecting many others. The following patch is inspired by David Laight's (dsl@NetBSD) r1.21. Instead of hardcoded values, use names for the bit patterns that indicate which values were specified for reps, begin, ender and step on the command line. Then use a simple switch statement to decide what to do and add two detailed comments that explain why we do what we do. Note that it makes absolutely no sense to compute anything if random output is requested, so just skip the whole switch in that case. Also try to be a bit more helpful with the error messages: The nonsensical "Infinite sequences cannot be bounded" does not really help with figuring out what went wrong. Index: jot.c =================================================================== RCS file: /var/cvs/src/usr.bin/jot/jot.c,v retrieving revision 1.31 diff -u -p -r1.31 jot.c --- jot.c 5 Aug 2016 13:43:38 -0000 1.31 +++ jot.c 6 Aug 2016 13:48:18 -0000 @@ -52,11 +52,16 @@ #define ENDER_DEF 100 #define STEP_DEF 1 +#define STEP 1 +#define ENDER 2 +#define BEGIN 4 +#define REPS 8 + #define is_default(s) (strcmp((s), "-") == 0) static double begin = BEGIN_DEF; static double ender = ENDER_DEF; -static double s = STEP_DEF; +static double step = STEP_DEF; static long reps = REPS_DEF; static bool randomize; static bool infinity; @@ -131,9 +136,9 @@ main(int argc, char *argv[]) switch (argc) { /* examine args right to left, falling thru cases */ case 4: if (!is_default(argv[3])) { - if (!sscanf(argv[3], "%lf", &s)) + if (!sscanf(argv[3], "%lf", &step)) errx(1, "Bad s value: %s", argv[3]); - mask |= 01; + mask |= STEP; if (randomize) warnx("random seeding not supported"); } @@ -141,7 +146,7 @@ main(int argc, char *argv[]) if (!is_default(argv[2])) { if (!sscanf(argv[2], "%lf", &ender)) ender = argv[2][strlen(argv[2])-1]; - mask |= 02; + mask |= ENDER; if (prec == -1) n = getprec(argv[2]); } @@ -149,7 +154,7 @@ main(int argc, char *argv[]) if (!is_default(argv[1])) { if (!sscanf(argv[1], "%lf", &begin)) begin = argv[1][strlen(argv[1])-1]; - mask |= 04; + mask |= BEGIN; if (prec == -1) prec = getprec(argv[1]); if (n > prec) /* maximum precision */ @@ -159,7 +164,7 @@ main(int argc, char *argv[]) if (!is_default(argv[0])) { if (!sscanf(argv[0], "%ld", &reps)) errx(1, "Bad reps value: %s", argv[0]); - mask |= 010; + mask |= REPS; if (prec == -1) prec = 0; } @@ -172,101 +177,74 @@ main(int argc, char *argv[]) argv[4]); } getformat(); - while (mask) /* 4 bit mask has 1's where last 4 args were given */ - switch (mask) { /* fill in the 0's by default or computation */ - case 001: - reps = REPS_DEF; - mask = 011; - break; - case 002: - reps = REPS_DEF; - mask = 012; - break; - case 003: - reps = REPS_DEF; - mask = 013; - break; - case 004: - reps = REPS_DEF; - mask = 014; - break; - case 005: - reps = REPS_DEF; - mask = 015; - break; - case 006: - s = ender > begin ? 1 : -1; - mask = 007; - /* FALLTHROUGH */ - case 007: - if (randomize) { - reps = REPS_DEF; - mask = 0; - break; + /* If random output is requested, use defaults for omitted values. */ + if (!randomize) { + /* + * Consolidate the values of reps, begin, ender, step: + * The formula ender - begin == (reps - 1) * step shows that any + * three determine the fourth (unless reps == 1 or step == 0). + * The manual states the following rules: + * 1. If four are specified, compare the given and the computed + * value of reps and take the smaller of the two. + * 2. If steps was omitted, it takes the default, unless both + * begin and ender were specified. + * 3. Assign defaults to omitted values for reps, begin, ender, + * from left to right. + * 4. reps == 0 means infinite output + */ + switch (mask) { /* Four cases involve both begin and ender. */ + case REPS | BEGIN | ENDER | STEP: + if (reps == 0) + errx(1, + "Can't specify end of infinite sequence"); + if (step != 0.0) { + long t = (ender - begin + step) / step; + if (t <= 0) + errx(1, "Impossible stepsize"); + if (t < reps) + reps = t; } - if (s == 0.0) { + break; + case REPS | BEGIN | ENDER: + if (reps == 0) + errx(1, + "Can't specify end of infinite sequence"); + if (reps == 1) + step = 0.0; + else + step = (ender - begin) / (reps - 1); + break; + case BEGIN | ENDER: + step = ender > begin ? 1 : -1; /* FreeBSD's behavior. */ + /* FALLTHROUGH */ + case BEGIN | ENDER | STEP: + if (step == 0.0) { reps = 0; - mask = 0; break; } - reps = (ender - begin + s) / s; + reps = (ender - begin + step) / step; if (reps <= 0) errx(1, "Impossible stepsize"); - mask = 0; - break; - case 010: - begin = BEGIN_DEF; - mask = 014; - break; - case 011: - begin = BEGIN_DEF; - mask = 015; - break; - case 012: - s = STEP_DEF; - mask = 013; - break; - case 013: - if (randomize) - begin = BEGIN_DEF; - else if (reps == 0) - errx(1, "Must specify begin if reps == 0"); - begin = ender - reps * s + s; - mask = 0; - break; - case 014: - s = STEP_DEF; - mask = 015; - break; - case 015: - if (randomize) - ender = ENDER_DEF; - else - ender = begin + reps * s - s; - mask = 0; break; - case 016: + case ENDER: /* Four cases involve only ender. */ + case ENDER | STEP: + case REPS | ENDER: + case REPS | ENDER | STEP: if (reps == 0) - errx(1, "Infinite sequences cannot be bounded"); - else if (reps == 1) - s = 0.0; - else - s = (ender - begin) / (reps - 1); - mask = 0; - break; - case 017: /* if reps given and implied, */ - if (!randomize && s != 0.0) { - long t = (ender - begin + s) / s; - if (t <= 0) - errx(1, "Impossible stepsize"); - if (t < reps) /* take lesser */ - reps = t; - } - mask = 0; + errx(1, + "Must specify start of infinite sequence"); + begin = ender - reps * step + step; break; default: - errx(1, "bad mask"); + /* + * The remaining eight cases omit ender. We don't need + * to compute anything because only reps, begin, step + * are used for producing output below. Rules 2. and 3. + * together imply that ender will be set last. + */ + break; } + } if (reps == 0) infinity = true; if (randomize) { @@ -315,14 +293,14 @@ main(int argc, char *argv[]) if (putdata(v, reps == i && !infinity)) errx(1, "range error in conversion: %f", v); } - } - else - for (i = 1, x = begin; i <= reps || infinity; i++, x += s) + } else { + for (i = 1, x = begin; i <= reps || infinity; i++, x += step) if (putdata(x, reps == i && !infinity)) errx(1, "range error in conversion: %f", x); + } if (finalnl) putchar('\n'); - exit(0); + return (0); } static int ----- End forwarded message -----
