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;

Reply via email to