Re: [Groff] PDFPIC macro

[Keith Marshall]

On 29/09/14 21:20, Werner LEMBERG wrote:
> 
>> I've refactored the appropriate code, in src/roff/troff/input.cpp,
>> with a view to accommodating this; see patch attached.  While I've
>> not yet progressed any implementation for PDF handling, I have
>> indicated the point at which it should be invoked.  Okay to commit,
>> thus far?
> 
> This is very nice, thanks!  For my taste, the comments are a bit
> excessive, but I guess this is probably only me who thinks so :-)

I've always favoured a verbose commenting style, since I learned to
write FORTRAN-66, way back in the early 1970s.  Today, with my failing
60+ year old memory, I really appreciate the value of this, when I come
back to code 6 months or so after I wrote it, and find that I cannot
remember how it was supposed to work :-)

> Some comments, mainly regarding formatting:
> 
> +           while ((context = bounding_box_args()) == NULL
> +           &&     get_line(DSC_LINE_MAX_ENFORCE) > 0)
> +             ;
> 
> Please say
> 
>             while ((context = bounding_box_args()) == NULL
>                    && get_line(DSC_LINE_MAX_ENFORCE) > 0)
>               ;

Okay.

> +      while (*context == '\x20' || *context == '\t')
> +             context++;

Hmm.  I don't know how that crept in ...

> Please say
> 
>       while (*context == '\x20' || *context == '\t')
>         context++;

This is how it *looked* in my working file copy, but there's one tab in
amongst a string of spaces there -- I must have somehow managed to
override vim's normal tab insertion on that line.  The extra '+' flag,
in the patch file, pushed it over by an extra tab stop, and I missed it
when I cast my eye over the patch.  It should be fixed, in the updated
patch attached.

> +      status = (context_args("(atend)", context) == NULL)
> +     ? llx = lly = urx = ury = PSBB_RANGE_IS_BAD
> +     : PSBB_RANGE_AT_END;
> 
> Please say
> 
>       status = (context_args("(atend)", context) == NULL)
>                ? llx = lly = urx = ury = PSBB_RANGE_IS_BAD
>                : PSBB_RANGE_AT_END;

Okay.

> +  register size_t len = strlen(tag);
> 
> Given that recent versions of clang emit warnings if it encounters the
> `register' keyword, and given that this keyword has no effect for
> about 20 years, please don't use it:

Done, but are you sure about the lack of effect?  (I seem to recall
having observed a benefit, quite recently, in assembly code generated by
GCC-4.8.2).

-- 
Regards,
Keith.

psbb-process.patch
Description: Text Data