Paul Eggert <egg...@cs.ucla.edu> writes:
> On 2025-07-28 21:39, Collin Funk wrote:
>
>> Thanks for again for the thorough review and explanations. I find it
>> funny that I assumed this change was simple and learned it's purpose was
>> for logging in on a teletype. The Model 37 predates me ~30 years, so it
>> never occured to me that the purpose was to not print logins.
>
> Heh, in the 1960s I used a Teletype Model 33, which was so bad that
> nobody ever wanted to print anything nice on it and 'pr' would have
> never gotten these flags if all people had were Model 33s. And that
> wasn't the worst device I used to write programs!
I'm curious, what is the worst?
>> +After printing each page, print an alert (bell) to standard error and
>> +wait for a newline to be read from @file{/dev/tty} before printing the
>> +next page.
>
> This sentence should start "Before" not "After", with the rest of the
> sentence changed accordingly. There is no bell printed after the last
> page.
>
> Looking at the code, it seems that this mistake has leaked into the
> code as well, in that sometimes 'pr' will print an alert and wait for
> '\n' at end of file, when there is no page to print. If I'm right,
> that needs to be fixed; pr should wait for '\n' only if it will
> actually print something afterward.
Good catch, I guess the logic of checking if there are more pages to
print should be hoisted out of 'print_page ()' or the pausing logic
should be handled there, just before printing. Let me see which one is
less of a chore to read.
>> + if (pause_option && close (tty_fd) < 0)
>> + error (EXIT_FAILURE, errno, "%s", quotef ("/dev/tty"));
>
> Why are these lines useful? As far as I can see they merely add
> complexity for no benefit. How about removing them? (If we kept them
> we would need to fix the bug in them; but let's remove them.)
I assume we would want to close the file descriptors that we open at the
end of the program. If so, I guess there is no point in checking for
errors from 'close'. Or is the close not nessecary, similar to calling
'free' on memory just before exit?
>> + if (putc ('\a', stderr) == EOF || fflush (stderr) != 0)
>
> fputc here; there's no need for efficiency since we're gonna fflush anyway.
>
> More important, this fputc and fflush should be skipped if
> pause_option is false.
That was my interpretation as well. But I noticed that FreeBSD and
NetBSD will print the alert using 'pr -f'. Since both of us agree that
is wrong, I will change it.
> You might want to turn this part of the code into a separate static
> function, for clarity. (Or you might not....)
Yep, at first the conditions were too small to benefit from an extra
function. But over time it is beginning to make more sense...
Thanks,
Collin
