[groff] 16/19: [troff]: Give font diagnostics more context.

[G. Branden Robinson]

gbranden pushed a commit to branch master
in repository groff.

commit d8cb8cf9d833b0a6afe525d11dc6a5ce242d1355
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
AuthorDate: Wed Sep 15 14:49:58 2021 +1000

    [troff]: Give font diagnostics more context.
    
    [troff]: Lift font mounting diagnostic messages to be closer to their
    user-controlled contexts to provide more information.  In many cases no
    diagnostic was being thrown at all when an unavailable font was
    requested by name, which is the method most users prefer, and which
    meant that failures resulting from typos in font names for many requests
    (`uf`, `fschar`, `rfschar`, `special`, `fspecial`, `fzoom`, `bd`, `tkf`,
    `cs`) were going unreported.  Also, these font warnings are promoted to
    errors because the request will utterly fail to do what was requested
    with no reasonable fallback.  Possibly, they were warnings in the first
    place because they could also be thrown regarding unavailable fonts
    encountered in device description files, and while that's bad news, it
    results in no formatting problems if it doesn't affect fonts that an
    input document actually uses; thus, a mere warning is appropriate.
    
    * src/roff/troff/node.cpp (struct font_lookup_info): New struct keeps
      the font name or position requested, and the position of successful
      font lookup.
    
      (font_lookup_info::font_lookup_info): Add constructor.
    
      (font_lookup_error): New function builds error message using a
      `font_lookup_info` struct and a message argument.
    
      (get_fontno): Rename to...
    
      (has_font): ...this.  Add argument to take a pointer to a
      `font_lookup_info` struct.  Return a `bool` indicating whether the
      lookup succeeded.  Place former `int` return value into the struct
      instead.  Populate the other struct members with the requested font
      name or position, as appropriate.
    
      (mount_font_no_translate): Stop throwing warning diagnostic here if a
      font cannot be loaded.  Instead, throw them...
    
      (font_position): ...here, and...
    
      (underline_font, define_font_special_character,
      remove_font_special_character, read_special_fonts,
      font_special_request, font_zoom_request, bold_font, track_kern,
      constant_space): ...here, using `font_lookup_info` structs and
      `has_font()`.
    
      (remove_font_special_character): Stop returning early if font lookup
      fails; it's gratuitously inconsistent with other similar functions
      (save one, which has a reason to be different).
    
      (define_font_special_character): Return early if font lookup fails and
      say why in a comment (we can't `skip_line()`).
    
    Input:
    
    .uf Z
    .uf 99
    .fschar Z \[co] COPYRIGHT
    .fschar 99 \[co] COPYRIGHT
    .rfschar Z \[co]
    .rfschar 99 \[co]
    .special Y
    .special 98
    .fspecial Z Y
    .fspecial 99 Y
    .fzoom Z
    .fzoom 99
    .bd Z 3
    .bd 99 3
    .bd S Z 3
    .bd S 99 3
    .bd 98 Z 3
    .bd 98 99 3
    .tkf Z
    .tkf 99
    .cs Z
    .cs 99
    
    groff 1.22.4 diagnostics:
    
    troff: ...:1: warning: can't find font 'Z'
    troff: ...:2: bad font number
    troff: ...:4: bad font number
    troff: ...:6: bad font number
    troff: ...:7: warning: can't find font 'Y'
    troff: ...:8: bad font number
    troff: ...:10: bad font number
    troff: ...:12: bad font number
    troff: ...:14: bad font number
    troff: ...:17: bad font number
    troff: ...:18: bad font number
    troff: ...:20: bad font number
    troff: ...:22: bad font number
    
    groff 1.23.0 diagnostics:
    
    troff: ...:1: error: cannot load font 'Z' to make it the underline font
    troff: ...:2: error: cannot load font at position 99 to make it the 
underline font
    troff: ...:3: error: cannot load font 'Z' to define font-specific fallback 
glyph
    troff: ...:4: error: cannot load font at position 99 to define 
font-specific fallback glyph
    troff: ...:5: error: cannot load font 'Z' to remove font-specific fallback 
glyph
    troff: ...:6: error: cannot load font at position 99 to remove 
font-specific fallback glyph
    troff: ...:7: error: cannot load font 'Y' to mark it as special
    troff: ...:8: error: cannot load font at position 98 to mark it as special
    troff: ...:9: error: cannot load font 'Z' to mark other fonts as special 
contingently upon it
    troff: ...:10: error: cannot load font at position 99 to mark other fonts 
as special contingently upon it
    troff: ...:11: error: cannot load font 'Z' to set a zoom factor for it
    troff: ...:12: error: cannot load font at position 99 to set a zoom factor 
for it
    troff: ...:13: error: cannot load font 'Z' for emboldening
    troff: ...:14: error: cannot load font at position 99 for emboldening
    troff: ...:15: error: cannot load font 'Z' for conditional emboldening
    troff: ...:16: error: cannot load font at position 99 for conditional 
emboldening
    troff: ...:17: error: cannot load font at position 98 for emboldening
    troff: ...:18: error: cannot load font at position 98 for emboldening
    troff: ...:19: error: cannot load font 'Z' for track kerning
    troff: ...:20: error: cannot load font at position 99 for track kerning
    troff: ...:21: error: cannot load font 'Z' for constant spacing
    troff: ...:22: error: cannot load font at position 99 for constant spacing
---
 ChangeLog               |  44 ++++++++++++
 src/roff/troff/node.cpp | 176 ++++++++++++++++++++++++++++++++----------------
 2 files changed, 161 insertions(+), 59 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index beefddd..65058bc 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,49 @@
 2021-09-15  G. Branden Robinson <g.branden.robinson@gmail.com>
 
+       [troff]: Lift font mounting diagnostic messages to be closer to
+       their user-controlled contexts to provide more information.  In
+       many cases no diagnostic was being thrown at all when an
+       unavailable font was requested by name, which is the method most
+       users prefer, and which meant that failures resulting from typos
+       in font names for many requests (`uf`, `fschar`, `rfschar`,
+       `special`, `fspecial`, `fzoom`, `bd`, `tkf`, `cs`) were going
+       unreported.  Also, these font warnings are promoted to errors
+       because the request will utterly fail to do what was requested
+       with no reasonable fallback.  Possibly, they were warnings in
+       the first place because they could also be thrown regarding
+       unavailable fonts encountered in device description files, and
+       while that's bad news, it results in no formatting problems if
+       it doesn't affect fonts that an input document actually uses;
+       thus, a mere warning is appropriate.
+
+       * src/roff/troff/node.cpp (struct font_lookup_info): New struct
+       keeps the font name or position requested, and the position of
+       successful font lookup.
+       (font_lookup_info::font_lookup_info): Add constructor.
+       (font_lookup_error): New function builds error message using a
+       `font_lookup_info` struct and a message argument.
+       (get_fontno): Rename to...
+       (has_font): ...this.  Add argument to take a pointer to a
+       `font_lookup_info` struct.  Return a `bool` indicating whether
+       the lookup succeeded.  Place former `int` return value into the
+       struct instead.  Populate the other struct members with the
+       requested font name or position, as appropriate.
+       (mount_font_no_translate): Stop throwing warning diagnostic here
+       if a font cannot be loaded.  Instead, throw them...
+       (font_position): ...here, and...
+       (underline_font, define_font_special_character,
+       remove_font_special_character, read_special_fonts,
+       font_special_request, font_zoom_request, bold_font, track_kern,
+       constant_space): ...here, using `font_lookup_info` structs and
+       `has_font()`.
+       (remove_font_special_character): Stop returning early if font
+       lookup fails; it's gratuitously inconsistent with other similar
+       functions (save one, which has a reason to be different).
+       (define_font_special_character): Return early if font lookup
+       fails and say why in a comment (we can't `skip_line()`).
+
+2021-09-15  G. Branden Robinson <g.branden.robinson@gmail.com>
+
        * [libgroff, troff]: Further boolify.
 
        * src/include/font.h (load_font, font): Demote parameters from
diff --git a/src/roff/troff/node.cpp b/src/roff/troff/node.cpp
index e0d7e5a..8300390 100644
--- a/src/roff/troff/node.cpp
+++ b/src/roff/troff/node.cpp
@@ -108,6 +108,18 @@ public:
   hunits compute(int point_size);
 };
 
+struct font_lookup_info {
+  int position;
+  int requested_position;
+  char *requested_name;
+  font_lookup_info();
+};
+
+font_lookup_info::font_lookup_info() : position(-1),
+  requested_position(-1), requested_name(0)
+{
+}
+
 // embolden fontno when this is the current font
 
 struct conditional_bold {
@@ -5946,9 +5958,6 @@ static bool mount_font_no_translate(int n, symbol name,
     if (check_only)
       return fm != 0;
     if (!fm) {
-      if (not_found)
-       warning(WARN_FONT, "can't find font '%1'",
-               external_name.contents());
       (void)font_dictionary.lookup(external_name, &a_char);
       return false;
     }
@@ -6045,7 +6054,9 @@ void font_position()
       symbol internal_name = get_name(true /* required */);
       if (!internal_name.is_null()) {
        symbol external_name = get_long_name();
-       (void) mount_font(n, internal_name, external_name);
+       if (!mount_font(n, internal_name, external_name))
+         error("cannot load font '%1' for mounting",
+               internal_name.contents());
       }
     }
   }
@@ -6156,38 +6167,54 @@ void style()
   skip_line();
 }
 
-static int get_fontno()
+static void font_lookup_error(font_lookup_info& finfo,
+                             const char *msg)
+{
+  if (finfo.requested_name)
+    error("cannot load font '%1' %2", finfo.requested_name, msg);
+  else
+    error("cannot load font at position %1 %2",
+         finfo.requested_position, msg);
+}
+
+// Read the next token and look it up as a font name or position number.
+// Return lookup success.  Store, in the supplied struct argument, the
+// requested name or position, and the position actually resolved; -1
+// means not found (see `font_lookup_info` constructor).
+static bool has_font(font_lookup_info *finfo)
 {
   int n;
   tok.skip();
   if (tok.usable_as_delimiter()) {
     symbol s = get_name(true /* required */);
+    finfo->requested_name = (char *)s.contents();
     if (!s.is_null()) {
       n = symbol_fontno(s);
       if (n < 0) {
        n = next_available_font_position();
-       if (!mount_font(n, s))
-         return -1;
+       if (mount_font(n, s))
+         finfo->position = n;
       }
-      return curenv->get_family()->make_definite(n);
+      finfo->position = curenv->get_family()->make_definite(n);
     }
   }
   else if (get_integer(&n)) {
-    if (n < 0 || n >= font_table_size || font_table[n] == 0)
-      error("bad font number");
-    else
-      return curenv->get_family()->make_definite(n);
+    finfo->requested_position = n;
+    if (!(n < 0 || n >= font_table_size || font_table[n] == 0))
+      finfo->position = curenv->get_family()->make_definite(n);
   }
-  return -1;
+  return (finfo->position != -1);
 }
 
 static int underline_fontno = 2;
 
 void underline_font()
 {
-  int n = get_fontno();
-  if (n >= 0)
-    underline_fontno = n;
+  font_lookup_info finfo;
+  if (!has_font(&finfo))
+    font_lookup_error(finfo, "to make it the underline font");
+  else
+    underline_fontno = finfo.position;
   skip_line();
 }
 
@@ -6198,38 +6225,43 @@ int get_underline_fontno()
 
 void define_font_special_character()
 {
-  int n = get_fontno();
-  if (n < 0) {
+  font_lookup_info finfo;
+  if (!has_font(&finfo)) {
+    font_lookup_error(finfo, "to define font-specific fallback glyph");
+    // Normally we skip the remainder of the line unconditionally at the
+    // end of a request-implementing function, but do_define_character()
+    // will eat the rest of it for us.
     skip_line();
-    return;
   }
-  symbol f = font_table[n]->get_name();
-  do_define_character(CHAR_FONT_SPECIAL, f.contents());
+  else {
+    symbol f = font_table[finfo.position]->get_name();
+    do_define_character(CHAR_FONT_SPECIAL, f.contents());
+  }
 }
 
 void remove_font_special_character()
 {
-  int n = get_fontno();
-  if (n < 0) {
-    skip_line();
-    return;
-  }
-  symbol f = font_table[n]->get_name();
-  while (!tok.is_newline() && !tok.is_eof()) {
-    if (!tok.is_space() && !tok.is_tab()) {
-      charinfo *s = tok.get_char(true /* required */);
-      string gl(f.contents());
-      gl += ' ';
-      gl += s->nm.contents();
-      gl += '\0';
-      charinfo *ci = get_charinfo(symbol(gl.contents()));
-      if (!ci)
-       break;
-      macro *m = ci->set_macro(0);
-      if (m)
-       delete m;
+  font_lookup_info finfo;
+  if (!has_font(&finfo))
+    font_lookup_error(finfo, "to remove font-specific fallback glyph");
+  else {
+    symbol f = font_table[finfo.position]->get_name();
+    while (!tok.is_newline() && !tok.is_eof()) {
+      if (!tok.is_space() && !tok.is_tab()) {
+       charinfo *s = tok.get_char(true /* required */);
+       string gl(f.contents());
+       gl += ' ';
+       gl += s->nm.contents();
+       gl += '\0';
+       charinfo *ci = get_charinfo(symbol(gl.contents()));
+       if (!ci)
+         break;
+       macro *m = ci->set_macro(0);
+       if (m)
+         delete m;
+      }
+      tok.next();
     }
-    tok.next();
   }
   skip_line();
 }
@@ -6245,10 +6277,12 @@ static void read_special_fonts(special_font_list **sp)
   }
   special_font_list **p = sp;
   while (has_arg()) {
-    int i = get_fontno();
-    if (i >= 0) {
+    font_lookup_info finfo;
+    if (!has_font(&finfo))
+      font_lookup_error(finfo, "to mark it as special");
+    else {
       special_font_list *tem = new special_font_list;
-      tem->n = i;
+      tem->n = finfo.position;
       tem->next = 0;
       *p = tem;
       p = &(tem->next);
@@ -6258,9 +6292,12 @@ static void read_special_fonts(special_font_list **sp)
 
 void font_special_request()
 {
-  int n = get_fontno();
-  if (n >= 0)
-    read_special_fonts(&font_table[n]->sf);
+  font_lookup_info finfo;
+  if (!has_font(&finfo))
+    font_lookup_error(finfo, "to mark other fonts as special"
+                            " contingently upon it"); // a mouthful :-/
+  else
+    read_special_fonts(&font_table[finfo.position]->sf);
   skip_line();
 }
 
@@ -6272,8 +6309,11 @@ void special_request()
 
 void font_zoom_request()
 {
-  int n = get_fontno();
-  if (n >= 0) {
+  font_lookup_info finfo;
+  if (!has_font(&finfo))
+    font_lookup_error(finfo, "to set a zoom factor for it");
+  else {
+    int n = finfo.position;
     if (font_table[n]->is_style())
       warning(WARN_FONT, "can't set zoom factor for a style");
     else {
@@ -6380,12 +6420,23 @@ hunits env_narrow_space_width(environment *env)
 
 void bold_font()
 {
-  int n = get_fontno();
-  if (n >= 0) {
+  font_lookup_info finfo;
+  if (!has_font(&finfo))
+    font_lookup_error(finfo, "for emboldening");
+  else {
+    int n = finfo.position;
     if (has_arg()) {
+      // This is a bit non-orthogonal, but faithful to CSTR #54.  We can
+      // only conditionally embolden a font specified by name, not
+      // position, so ".bd S B 4" works but ".bd 5 3 4" does not.  The
+      // latter bolds the font at position 5 unconditionally, and
+      // ignores the third argument.
       if (tok.usable_as_delimiter()) {
-       int f = get_fontno();
-       if (f >= 0) {
+      font_lookup_info finfo2;
+       if (!has_font(&finfo2))
+         font_lookup_error(finfo2, "for conditional emboldening");
+       else {
+         int f = finfo2.position;
          units offset;
          if (has_arg() && get_number(&offset, 'u') && offset >= 1)
            font_table[f]->set_conditional_bold(n, hunits(offset - 1));
@@ -6394,6 +6445,9 @@ void bold_font()
        }
       }
       else {
+       font_lookup_info finfo2;
+         if (!has_font(&finfo2))
+           font_lookup_error(finfo2, "for conditional emboldening");
        units offset;
        if (get_number(&offset, 'u') && offset >= 1)
          font_table[n]->set_bold(hunits(offset - 1));
@@ -6461,9 +6515,11 @@ hunits track_kerning_function::compute(int size)
 
 void track_kern()
 {
-  int n = get_fontno();
-  if (n >= 0) {
-    int min_s, max_s;
+  font_lookup_info finfo;
+  if (!has_font(&finfo))
+    font_lookup_error(finfo, "for track kerning");
+  else {
+    int n = finfo.position, min_s, max_s;
     hunits min_a, max_a;
     if (has_arg()
        && get_number(&min_s, 'z')
@@ -6483,9 +6539,11 @@ void track_kern()
 
 void constant_space()
 {
-  int n = get_fontno();
-  if (n >= 0) {
-    int x, y;
+  font_lookup_info finfo;
+  if (!has_font(&finfo))
+    font_lookup_error(finfo, "for constant spacing");
+  else {
+    int n = finfo.position, x, y;
     if (!has_arg() || !get_integer(&x))
       font_table[n]->set_constant_space(CONSTANT_SPACE_NONE);
     else {