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
