Re: [Groff] PDFPIC macro

[Ralph Corderoy]

Hi Keith,

First off, thanks for putting the effort in;  you're one of the few
who's doing that and code wins over codeless argument.  That said, I've
some complaints that may sway your style.  :-)

In general, I dislike the verbose comments, both in number and
wordiness.  Also, vertical space is precious as it allows me to glance
up and down without the distraction of scrolling;  the many empty `//'
comments waste this and pull the comment from its subject line.

Yes, comments are useful, as Werner said, but we've better control-flow
structures than early Fortran, break and continue are welcome, a
function need not clutter its control flow by steering towards a single
return, goto is even useful in common idioms.  And we're not limited to
two-letter identifiers like troff, so comments are not as much needed as
they were.  Especially with well-chosen identifiers;  I find
context_args() non-obvious.

Taking a bit at random...

>     // psbb_locator class constructor.
>     //

Not needed.

>     psbb_locator::psbb_locator(const char *fname):
>     filename(fname), llx(0), lly(0), urx(0), ury(0), lastc(EOF)
>     {
>       // PS files might contain non-printable characters, such as ^Z
>       // and CRs not followed by an LF, so open them in binary mode.
>       //
>       fp = include_search_path.open_file_cautious(filename, 0, FOPEN_RB);

We know it's a PostScript file.  More succinctly,

        // Binary mode as, for example, ^Z and CR with LF are valid.
        fp = include_search_path.open_file_cautious(filename, 0, FOPEN_RB);

>       if (fp) {
>         // After successfully opening the file, acquire the first
>         // line, whence we may determine the file format...
>         //
>         if (get_line(DSC_LINE_MAX_ENFORCE) == 0)

This is describing straightforward control flow.  `if fp' means we
successfully opened the file;  the reader is expect to know that.  He
should also know that what happens in the then-block is what happens
next, `After' not needed.  And what happens next it to get_line(), which
is to get a line;  it's not surprising that it's the first line as we've
just opened the file.

>           //
>           // ...except in the case of an empty file, which we are
>           // unable to process further.
>           //
>           error("`%1' is empty", filename);

The textual error message tells me what the problem is.  I don't need
that comment;  or if I do then the error message is unclear to the
reader of the code and the user that experiences it.  :-) Control flow
shows it's not processed further, *if* I follow through all the rest of
the function.

BTW, I dislike having to mentally stack many if-conditions simply so all
the elses do a simple thing, like error().  Especially when those elses
are off-screen.  I have to stack them to see if an else is present each
time and what it's the else of.  And that means moving to the matching
brace, which is off-screen because too much vertical space is wasted.  I
think I mentioned that.  :-)

Here's a non-compiled attempt at that function.  I don't suppose you'll
favour it any more than I'm tempted to write in your style, but I hope
it gets my points across.

    psbb_locator::psbb_locator(const char *fname):
    filename(fname), llx(0), lly(0), urx(0), ury(0), lastc(EOF)
    {
        // Binary mode as, for example, ^Z and CR with LF are valid.
        fp = include_search_path.open_file_cautious(filename, 0, FOPEN_RB);
        if (!fp) {
            error("can't open `%1': %2", filename, strerror(errno));
            goto done;
        }

        if (!get_line(DSC_LINE_MAX_ENFORCE)) {
            error("`%1' is empty", filename);
            goto done;
        }

        if (!expect("%!PS-Adobe-")) {
            error("`%1' does not conform to the Document Structuring 
Conventions",
                  filename);
            goto done;
        }

        const char *s = NULL;
        while (get_header_comment() && !(s = match("%%BoundingBox:")))
            ;
        if (!s) {
            error("%%%%BoundingBox comment not found in headers: `%1'", 
filename);
            goto done;
        }

        int status = parse_bounding_box(s);
        if (status == PSBB_RANGE_IS_SET)
            goto done;
        if (status == PSBB_RANGE_IS_BAD)
            goto bad_range;

        // status is PSBB_RANGE_AT_END, retrieve and check that one.
        if (!skip_to_trailer()) {
            error("%%%%BoundingBox at end, but no %%%%Trailer: `%1'", filename);
            goto done;
        }

        s = NULL;
        while (get_line(DSC_LINE_MAX_ENFORCE) && !(s = match("%%BoundingBox:")))
            ;
        if (!s) {
            error("%%%%BoundingBox comment not found at end: `%1'", filename);
            goto done;
        }

        status = parse_bounding_box(s);
        if (status == PSBB_RANGE_IS_SET)
            goto done;
        if (status == PSBB_RANGE_AT_END) {
            error("%%%%BoundingBox in %%%%Trailer also says at end: `%1'", 
filename);
            goto done;
        }
        // status must be...

    bad_range:
        error("invalid arguments to %%%%BoundingBox in `%1': %2", filename, s);

    done:
        if (fp)
            fclose(fp);
        assign_registers();   // Use defaults if no valid %%BoundingBox.
    }

Yes, I have to scroll down almost immediately to find `done', but it's
short, I can quickly glean it's purpose, and all the other times I come
across it, I know it's clean-up on function exit without having to
look again;  it's not much to remember compared to all those if-elses.

> +inline int psbb_locator::skip_to_trailer(void)
> +{
> +  // Begin by considering a chunk of the input file starting 512 bytes
> +  // before its end, and search it for a "%%Trailer" comment; if none is
> +  // found, incrementally double the chunk size while it remains within
> +  // a 32768L byte range, and search again...
> +  //
> +  for (ssize_t offset = 512L; offset > 0L; offset <<= 1) {

This scans the last 512, 1KiB, 2KiB, 4KiB, 8KiB, 16KiB, and 32KiB of the
file before giving up, after 63KiB of work, and scanning the whole file.
Rather than keep scanning over old ground, could it not just start 32KiB
from the end?

Cheers, Ralph.