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

[G. Branden Robinson]

Hi Lennart,

I composed this back in February but postponed it, and I don't know
remember why.

At 2024-02-26T14:40:53+0000, Lennart Jablonka wrote:
> 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.)

No worries.

> > +    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.

Yup.  You can tell I'm old, instinctively using an `int` instead of a
proper type.

> > +    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,

The last is not desirable, I think, until and unless we overhaul groff
to drop its own string class and use std::string _everywhere_.  I
shudder at the potential horrors of mixing up objects of these two
classes.

Apart from that, I dither over when and where to use C strings vs. groff
strings in the code.  I have groped in vain for an organizing principle
that would tell me, apart from "I need to call a standard libc function
that expects a C string".

> 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=%)

Insufficient attention on my part, most likely.

> instead of doing:
>
>       fatal("entry for '%1' not found in terminal database", terminal_type);

I'm sure I thought I had a reason at the time, but I don't recollect it
now.

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

That's not up to grotty.  `fatal()` is defined by libgroff and I would
prefer to land this change with no disturbance to any other part of the
system.  It should be easier to detect and remedy regressions that way.

https://git.savannah.gnu.org/cgit/groff.git/tree/src/libs/libgroff/error.cpp?h=1.23.0#n123

> > +  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.

1.  I prefer narrow interfaces to broad ones;
2.  It seems clear enough;
3.  It communicates the type of the capability.

On the other hand, I also prefer communicative identifiers to cryptic
ones, and even terminfo codes (_usually_ limited to 5 characters) are on
the cryptic side.

So, not a hill I mean to die on.

> > +    // 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.

Good catch--thank you!

Regards,
Branden

signature.asc
Description: PGP signature