[groff] 14/15: [libgroff, troff]: Slightly refactor.

[G. Branden Robinson]

gbranden pushed a commit to branch master
in repository groff.

commit 2dff87d3edba3c82f93dd4b07a95bb42543a5d86
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
AuthorDate: Fri Sep 17 18:56:08 2021 +1000

    [libgroff, troff]: Slightly refactor.
    
    [libgroff, troff]: Slightly refactor device and font description file
    loading.  Remove dead code.
    
    * src/include/font.h (font::load_font): Drop second parameter.  It was
      never used for its intended purpose.
    
      (load): Drop first parameter; likewise.
    
    * src/libs/libgroff/font.cpp (font::load)
      (load): As above.
    
    * src/libs/libgroff/font.cpp (struct text_file): Rename `skip_comments`
      to `recognize_comments`.  Demote that and `silent` from `int` to
      `bool`.
    
      (text_file::text_file): Use Boolean rather than integer literals in
      constructor.
    
      (text_file::next_line, font::load): Apply above renaming.
    
      * src/libs/libgroff/font.cpp (font::load): Rename parameter from
      `head_only` to `load_header_only` to be more communicative.  Drop test
      of font description file name being `DESC`; this code was not being
      reached.  Stop throwing errors from this function on failure to open
      the file; the caller will handle this when it sees our false return
      value.  Rename local variable `command` to `directive` for alignment
      with our documentation.  Replace "I dont think this should happen"
      test and comment with `assert()`.
    
      (struct table): Rename member from `command` to `numeric_directive` to
      indicate its specificity, tracking only a subset of valid `DESC` file
      directives.
    
      * src/libs/libgroff/font.cpp (font::load, font::load_desc): Remove
      redundant assignments to the member variable formerly known as
      `skip_comments`.
    
      * src/libs/libgroff/font.cpp (font::load_desc): Rename local variable
      `directive_found` to `numeric_directive_found` to clarify logic.
    
      * src/roff/troff/node.cpp (mount_font_no_translate): Simplify call of
      `font::load_font`.  The `not_found` in-out parameter which was so
      agonizingly passed up through layers of library calls was never
      actually read.  Drop code that has been `#if 0`-ed out since 1993.
    
    Before and after of goofing up by trying to mount a font called "DESC".
    The former is groff 1.22.4.
    
    $ groff
    .fp 5 DESC
    troff: <standard input>:1: warning: can't find font 'DESC'
    
    $ ./build/test-groff
    .fp 5 DESC
    troff: .../build/font/devps/DESC:15: error: unrecognized directive 'lpr' 
after 'kernpairs' or 'charset' directive
    troff: <standard input>:1: error: cannot load font 'DESC' for mounting
---
 ChangeLog                  | 44 +++++++++++++++++++++++++
 src/include/font.h         | 27 ++++++----------
 src/libs/libgroff/font.cpp | 80 +++++++++++++++++++---------------------------
 src/roff/troff/node.cpp    |  9 ++----
 4 files changed, 88 insertions(+), 72 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index b49fbb7..0c965d4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,49 @@
 2021-09-17  G. Branden Robinson <g.branden.robinson@gmail.com>
 
+       [libgroff, troff]: Slightly refactor device and font description
+       file loading.  Remove dead code.
+
+       * src/include/font.h (font::load_font): Drop second parameter.
+       It was never used for its intended purpose.
+       (load): Drop first parameter; likewise.
+       * src/libs/libgroff/font.cpp (font::load)
+       (load): As above.
+
+       * src/libs/libgroff/font.cpp (struct text_file): Rename
+       `skip_comments` to `recognize_comments`.  Demote that and
+       `silent` from `int` to `bool`.
+       (text_file::text_file): Use Boolean rather than integer literals
+       in constructor.
+       (text_file::next_line, font::load): Apply above renaming.
+
+       * src/libs/libgroff/font.cpp (font::load): Rename parameter from
+       `head_only` to `load_header_only` to be more communicative.
+       Drop test of font description file name being `DESC`; this code
+       was not being reached.  Stop throwing errors from this function
+       on failure to open the file; the caller will handle this when it
+       sees our false return value.  Rename local variable `command` to
+       `directive` for alignment with our documentation.  Replace "I
+       dont think this should happen" test and comment with `assert()`.
+       (struct table): Rename member from `command` to
+       `numeric_directive` to indicate its specificity, tracking only a
+       subset of valid `DESC` file directives.
+
+       * src/libs/libgroff/font.cpp (font::load, font::load_desc):
+       Remove redundant assignments to the member variable formerly
+       known as `skip_comments`.
+
+       * src/libs/libgroff/font.cpp (font::load_desc): Rename local
+       variable `directive_found` to `numeric_directive_found` to
+       clarify logic.
+
+       * src/roff/troff/node.cpp (mount_font_no_translate): Simplify
+       call of `font::load_font`.  The `not_found` in-out parameter
+       which was so agonizingly passed up through layers of library
+       calls was never actually read.  Drop code that has been `#if
+       0`-ed out since 1993.
+
+2021-09-17  G. Branden Robinson <g.branden.robinson@gmail.com>
+
        * src/libs/libgroff/font.cpp (font::load_desc): Clear line
        number before emitting whole-file validity diagnostics.
 
diff --git a/src/include/font.h b/src/include/font.h
index 62fb319..f0e294a 100644
--- a/src/include/font.h
+++ b/src/include/font.h
@@ -200,17 +200,13 @@ public:
                        // arg1).  Return the name of the size (in arg2),
                        // and the length and width (in arg3 and arg4).
                        // Return 1 in case of success, 0 otherwise.
-  static font *load_font(const char *, int * = 0, bool = false); // Load
-                       // the font description file with the given name
-                       // (arg1) and return it as a 'font' class.  If
-                       // arg2 points to an integer variable, set it to
-                       // 1 if the file is not found, without emitting
-                       // an error message.  If arg2 is NULL, print an
-                       // error message if the file is not found.  If
-                       // arg3 is true, only the part of the font
+  static font *load_font(const char *, bool = false); // Load the font
+                       // description file with the given name (arg1)
+                       // and return a pointer to a 'font' object.  If
+                       // arg2 is true, only the part of the font
                        // description file before the 'charset' and
-                       // 'kernpairs' sections is loaded.  Return NULL
-                       // in case of failure.
+                       // 'kernpairs' sections is loaded.  Return null
+                       // pointer in case of failure.
   static void command_line_font_dir(const char *);     // Prepend given
                        // path (arg1) to the list of directories in which
                        // to look up fonts.
@@ -331,13 +327,10 @@ protected:
   font(const char *);  // Initialize a font with the given name.
 
   // Load the font description file with the name in member variable
-  // `name` into this object.  If arg1 points to an integer variable,
-  // set it to 1 if the file is not found and do not emit an error
-  // message.  If arg1 is 0, print an error message if the file is not
-  // found.  If arg2 is true, only the part of the font description
-  // file before the 'charset' and 'kernpairs' sections is loaded.
-  // Return success/failure status of load.
-  bool load(int * = 0, bool = false);
+  // `name` into this object.  If arg1 is true, only the part of the
+  // font description file before the 'charset' and 'kernpairs' sections
+  // is loaded.  Return success/failure status of load.
+  bool load(bool = false);
 };
 
 // Local Variables:
diff --git a/src/libs/libgroff/font.cpp b/src/libs/libgroff/font.cpp
index 3eea8e1..a501c71 100644
--- a/src/libs/libgroff/font.cpp
+++ b/src/libs/libgroff/font.cpp
@@ -69,8 +69,8 @@ struct text_file {
   char *path;
   int lineno;
   int size;
-  int skip_comments;
-  int silent;
+  bool recognize_comments;
+  bool silent;
   char *buf;
   text_file(FILE *fp, char *p);
   ~text_file();
@@ -82,7 +82,7 @@ struct text_file {
 };
 
 text_file::text_file(FILE *p, char *s) : fp(p), path(s), lineno(1),
-  size(0), skip_comments(1), silent(0), buf(0)
+  size(0), recognize_comments(true), silent(false), buf(0)
 {
 }
 
@@ -130,7 +130,7 @@ bool text_file::next_line()
     char *ptr = buf;
     while (csspace(*ptr))
       ptr++;
-    if (*ptr != 0 && (!skip_comments || *ptr != '#'))
+    if (*ptr != 0 && (!recognize_comments || *ptr != '#'))
       return true;
   }
   return false;
@@ -681,10 +681,10 @@ void font::copy_entry(glyph *new_glyph, glyph *old_glyph)
   ch_index[new_index] = ch_index[old_index];
 }
 
-font *font::load_font(const char *s, int *not_found, bool head_only)
+font *font::load_font(const char *s, bool load_header_only)
 {
   font *f = new font(s);
-  if (!f->load(not_found, head_only)) {
+  if (!f->load(load_header_only)) {
     delete f;
     return 0;
   }
@@ -758,29 +758,14 @@ again:
   return 0;
 }
 
-// If the font can't be found, then if not_found is a valid pointer, its
-// referent will be set to 1, otherwise a message will be printed.
-bool font::load(int *not_found, bool head_only)
+bool font::load(bool load_header_only)
 {
-  if (strcmp(name, "DESC") == 0) {
-    if (not_found)
-      *not_found = 1;
-    else
-      error("'DESC' is not a valid font description file name");
-    return false;
-  }
   FILE *fp;
   char *path;
-  if ((fp = open_file(name, &path)) == 0) {
-    if (not_found)
-      *not_found = 1;
-    else
-      error("cannot open font description file '%1'", name);
+  if ((fp = open_file(name, &path)) == 0)
     return false;
-  }
   text_file t(fp, path);
-  t.skip_comments = 1;
-  t.silent = head_only;
+  t.silent = load_header_only;
   char *p = 0;
   while (t.next_line()) {
     p = strtok(t.buf, WS);
@@ -849,9 +834,9 @@ bool font::load(int *not_found, bool head_only)
       special = true;
     }
     else if (strcmp(p, "kernpairs") != 0 && strcmp(p, "charset") != 0) {
-      char *command = p;
+      char *directive = p;
       p = strtok(0, "\n");
-      handle_unknown_font_command(command, trim_arg(p), t.path, t.lineno);
+      handle_unknown_font_command(directive, trim_arg(p), t.path, t.lineno);
     }
     else
       break;
@@ -864,15 +849,15 @@ bool font::load(int *not_found, bool head_only)
     }
   }
   else {
-    char *command = p;
-    t.skip_comments = 0;
-    while (command) {
-      if (strcmp(command, "kernpairs") == 0) {
-       if (head_only)
+    char *directive = p;
+    t.recognize_comments = false;
+    while (directive) {
+      if (strcmp(directive, "kernpairs") == 0) {
+       if (load_header_only)
          return true;
        for (;;) {
          if (!t.next_line()) {
-           command = 0;
+           directive = 0;
            break;
          }
          char *c1 = strtok(t.buf, WS);
@@ -880,7 +865,7 @@ bool font::load(int *not_found, bool head_only)
            continue;
          char *c2 = strtok(0, WS);
          if (0 == c2) {
-           command = c1;
+           directive = c1;
            break;
          }
          p = strtok(0, WS);
@@ -898,22 +883,21 @@ bool font::load(int *not_found, bool head_only)
          add_kern(g1, g2, n);
        }
       }
-      else if (strcmp(command, "charset") == 0) {
-       if (head_only)
+      else if (strcmp(directive, "charset") == 0) {
+       if (load_header_only)
          return true;
        had_charset = true;
        glyph *last_glyph = 0;
        for (;;) {
          if (!t.next_line()) {
-           command = 0;
+           directive = 0;
            break;
          }
          char *nm = strtok(t.buf, WS);
-         if (nm == 0)
-           continue;                   // I dont think this should happen
+         assert(nm != 0);
          p = strtok(0, WS);
          if (0 == p) {
-           command = nm;
+           directive = nm;
            break;
          }
          if (p[0] == '"') {
@@ -1022,7 +1006,7 @@ bool font::load(int *not_found, bool head_only)
 }
 
 static struct {
-  const char *command;
+  const char *numeric_directive;
   int *ptr;
 } table[] = {
   { "res", &font::res },
@@ -1045,17 +1029,17 @@ bool font::load_desc()
   if ((fp = open_file("DESC", &path)) == 0)
     return false;
   text_file t(fp, path);
-  t.skip_comments = 1;
   res = 0;
   while (t.next_line()) {
     char *p = strtok(t.buf, WS);
-    bool directive_found = false;
+    assert(p != 0);
+    bool numeric_directive_found = false;
     unsigned int idx;
-    for (idx = 0; !directive_found
+    for (idx = 0; !numeric_directive_found
                  && idx < sizeof(table) / sizeof(table[0]); idx++)
-      if (strcmp(table[idx].command, p) == 0)
-       directive_found = true;
-    if (directive_found) {
+      if (strcmp(table[idx].numeric_directive, p) == 0)
+       numeric_directive_found = true;
+    if (numeric_directive_found) {
       char *q = strtok(0, WS);
       if (0 == q) {
        t.error("missing value for directive '%1'", p);
@@ -1232,9 +1216,9 @@ bool font::load_desc()
     else if (strcmp("charset", p) == 0)
       break;
     else if (unknown_desc_command_handler) {
-      char *command = p;
+      char *directive = p;
       p = strtok(0, "\n");
-      (*unknown_desc_command_handler)(command, trim_arg(p), t.path,
+      (*unknown_desc_command_handler)(directive, trim_arg(p), t.path,
                                      t.lineno);
     }
   }
diff --git a/src/roff/troff/node.cpp b/src/roff/troff/node.cpp
index 8300390..4e9fe34 100644
--- a/src/roff/troff/node.cpp
+++ b/src/roff/troff/node.cpp
@@ -5946,15 +5946,13 @@ static bool mount_font_no_translate(int n, symbol name,
                                    bool check_only = false)
 {
   assert(n >= 0);
-  // We store the address of this char in font_dictionary to indicate
+  // We store the address of this char in `font_dictionary` to indicate
   // that we've previously tried to mount the font and failed.
   static char a_char;
   font *fm = 0;
   void *p = font_dictionary.lookup(external_name);
   if (p == 0) {
-    int not_found;
-    fm = font::load_font(external_name.contents(), &not_found,
-                        check_only);
+    fm = font::load_font(external_name.contents(), check_only);
     if (check_only)
       return fm != 0;
     if (!fm) {
@@ -5964,9 +5962,6 @@ static bool mount_font_no_translate(int n, symbol name,
     (void)font_dictionary.lookup(name, fm);
   }
   else if (p == &a_char) {
-#if 0
-    error("invalid font '%1'", external_name.contents());
-#endif
     return false;
   }
   else