Re: [PATCH v3] [grotty]: Use terminfo.

[Lennart Jablonka]

Quoth G. Branden Robinson:
> I'm attaching my progress so far.

Nice!  I guess I’ll drop a few comments on it.  (Uncommented hunks 
omitted.)

> --- a/src/devices/grotty/tty.cpp
> +++ b/src/devices/grotty/tty.cpp
> @@ -916,8 +920,86 @@ printer *make_printer()
>   return new tty_printer();
> }
>
> -static void update_options()
> +static void initialize_terminal()
> {
> +  int err;
> +
> +  char *terminal_type = strdup(getenv("TERM"));
> +  int ret = setupterm(NULL /* use TERM */, STDOUT_FILENO, &err);
> +  size_t len = strlen(terminal_type);
> +
> +  if (len == 0)
> +      fatal("TERM environment variable must be set; aborting");
> +
> +  if (OK == ret) {
> +    ;
> +  }
> +  else if (ERR == ret) {
> +    const char msg1[] = "entry for '";
> +    const char msg2[] = "' not found in terminal database";
> +    const int bufsz = len + sizeof msg1 + sizeof msg2 + 1 /* `\0` */;

bufsz should be size_t.  len is size_t, the sizeof expressions are 
size_t, and calloc takes size_t.

> +    char *errbuf = static_cast<char *>(calloc(bufsz, sizeof (char)));
> +    (void) snprintf(errbuf, bufsz, "%s%s%s", msg1, terminal_type, msg2);

You could simplify that string handling using any of
  • snprintf(malloc((size = snprintf(NULL, 0, …))), size, …),
  • asprintf,
  • the string class,
  • the std::string class,
but …

> +    const char no_database[] = "terminfo database entry not found";
> +
> +    switch (err) {
> +    case -1:
> +      fatal(no_database);
> +      break;
> +    case 0:
> +      fatal(errbuf);

Did I miss something on why you’re allocating the error buffer 
even if never used (err == 1), not freeing it in that case, and 
introducing a bug à la printf(user_input) (when TERM=%), instead 
of doing:

        fatal("entry for '%1' not found in terminal database", terminal_type);

For the record, I think fatal() should be a simple wrapper for 
vfprintf.

> +    case 1:
> +      // POSIX calls this case "Success".  As an extension, to ncurses
> +      // it means a hardcopy terminal.  While fatal for many terminfo
> +      // applications, we're fine with it.
> +      break;
> +    default:
> +      fatal("terminfo interface is nonstandard; aborting");
> +    }
> +  }
> +  else
> +    fatal("terminfo interface is nonstandard; aborting");
> +
> +  if (tigetflag("hc")) {

Why are you using tigetflag instead of accessing the long names 
defined by term.h?  They do seem a little underspecified by 
X/Open Curses, but do exist, both there, somewhat, and in the 
implementations.

> +    // hardcopy terminal
> +    do_sgr_italics = false;
> +    do_reverse_video = false;
> +
> +    if (want_sgr_italics) {
> +      if (want_capability_warnings)
> +       warning("terminal type %1 is incapable of SGR italics",
> +               terminal_type);

Above, you 'quoted' terminal_type; here, you don’t.

> +      want_sgr_italics = false;
> +    }
> +  }
> +  else
> +    if (want_sgr_italics)
> +      if ((tigetstr("sitm") == 0 /* nullptr */)
> +          || (tigetstr("ritm") == 0 /* nullptr */)) {
> +       if (want_capability_warnings)
> +         warning("terminal type %1 is incapable of SGR italics",
> +                 terminal_type);
> +       want_sgr_italics = false;
> +      }
> +
> +  // X/Open Curses Issue 7 defines "os" as "Terminal overstrikes on
> +  // hard-copy terminal".  However, ncurses's terminfo database defines
> +  // it for graphical terminals as well; see entries for wy99gt-tek,
> +  // wy370-tek, tek, tek4013, gt40, and so on.  We therefore regard it
> +  // as generally relevant.
> +  if (tigetflag("os")) {
> +    // can overstrike
> +    do_bold = want_emboldening_by_overstriking;
> +    do_underline = want_italics_by_underlining;
> +  }
> +  else
> +    if (want_emboldening_by_overstriking
> +       || want_italics_by_underlining
> +       || want_glyph_composition_by_overstriking)
> +      if (want_capability_warnings)
> +       warning("terminal type %1 is incapable of overstriking",
> +               terminal_type);
> +
>   if (use_overstriking_drawing_scheme) {
>     do_sgr_italics = false;
>     do_reverse_video = false;