This one reads a lot better to me and even shaves of 2LoC. :-) OK martijn@
On Mon, 2020-11-30 at 18:28 -0600, Scott Cheloha wrote: > Hi, > > The cook_args() and raw_args() functions in cat(1) are too clever. > They handle multiple special cases in a single big loop with lots of > branches. It's been like this since at least 1989: > > https://svnweb.freebsd.org/csrg/bin/cat/cat.c?view=markup&pathrev=37179 > > The goal seems to be to avoid calling cook_buf()/raw_cat() from > multiple places. I think the result is convoluted. If we isolated > the special cases and called cook_buf()/raw_cat() from multiple places > the result would be simpler and flatter. > > You can break the cleanup in each function into four steps: > > 1. Pull the no-args case out of the loop and handle it first. Now we > don't need to check if (*argv == NULL) in the body of the loop. One > fewer assignment to fp/fd, too. > > 2. In the loop, isolate the (strcmp(*argv, "-") == 0) special case > from the normal filename case. Now we don't need to check whether > we're working with stdin when we clean up at the end of a loop > iteration. Setup and cleanup are adjacent, no additional branches > needed. > > 3. Pass the file name as an argument to cook_buf() and raw_cat(). > Now we don't need the global 'filename' variable. Obviously > this means we don't need to assign it a value, either. > > 4. Use a for-loop and move argv iteration into the loop header. > Now we increment argv in a single place in the loop. > > Thoughts? > > Index: cat.c > =================================================================== > RCS file: /cvs/src/bin/cat/cat.c,v > retrieving revision 1.27 > diff -u -p -r1.27 cat.c > --- cat.c 28 Jun 2019 13:34:58 -0000 1.27 > +++ cat.c 1 Dec 2020 00:24:20 -0000 > @@ -51,12 +51,11 @@ extern char *__progname; > > int bflag, eflag, nflag, sflag, tflag, vflag; > int rval; > -char *filename; > > void cook_args(char *argv[]); > -void cook_buf(FILE *); > +void cook_buf(FILE *, const char *); > void raw_args(char *argv[]); > -void raw_cat(int); > +void raw_cat(int, const char *); > > int > main(int argc, char *argv[]) > @@ -110,30 +109,29 @@ cook_args(char **argv) > { > FILE *fp; > > - fp = stdin; > - filename = "stdin"; > - do { > - if (*argv) { > - if (!strcmp(*argv, "-")) > - fp = stdin; > - else if ((fp = fopen(*argv, "r")) == NULL) { > - warn("%s", *argv); > - rval = 1; > - ++argv; > - continue; > - } > - filename = *argv++; > - } > - cook_buf(fp); > - if (fp == stdin) > + if (*argv == NULL) { > + cook_buf(stdin, "stdin"); > + return; > + } > + > + for (; *argv != NULL; argv++) { > + if (!strcmp(*argv, "-")) { > + cook_buf(stdin, "stdin"); > clearerr(fp); > - else > - (void)fclose(fp); > - } while (*argv); > + continue; > + } > + if ((fp = fopen(*argv, "r")) == NULL) { > + warn("%s", *argv); > + rval = 1; > + continue; > + } > + cook_buf(fp, *argv); > + (void)fclose(fp); > + } > } > > void > -cook_buf(FILE *fp) > +cook_buf(FILE *fp, const char *filename) > { > int ch, gobble, line, prev; > > @@ -200,28 +198,28 @@ raw_args(char **argv) > { > int fd; > > - fd = fileno(stdin); > - filename = "stdin"; > - do { > - if (*argv) { > - if (!strcmp(*argv, "-")) > - fd = fileno(stdin); > - else if ((fd = open(*argv, O_RDONLY, 0)) == -1) { > - warn("%s", *argv); > - rval = 1; > - ++argv; > - continue; > - } > - filename = *argv++; > + if (*argv == NULL) { > + raw_cat(fileno(stdin), "stdin"); > + return; > + } > + > + for(; *argv != NULL; argv++) { > + if (!strcmp(*argv, "-")) { > + raw_cat(fileno(stdin), "stdin"); > + continue; > + } > + if ((fd = open(*argv, O_RDONLY, 0)) == -1) { > + warn("%s", *argv); > + rval = 1; > + continue; > } > - raw_cat(fd); > - if (fd != fileno(stdin)) > - (void)close(fd); > - } while (*argv); > + raw_cat(fd, *argv); > + (void)close(fd); > + } > } > > void > -raw_cat(int rfd) > +raw_cat(int rfd, const char *filename) > { > int wfd; > ssize_t nr, nw, off; >
