Re: [Groff] PDFPIC macro

[Keith Marshall]

On 30/09/14 07:34, Werner LEMBERG wrote:
> 
>>> 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 :-)
> 
> Yeah.  Comments are good, but your new piece of code is about 10 times
> as verbose as the rest of groff...  And sometimes it's *really* too
> verbose for my taste.  For example, comments like
> 
>   // Member variables, shared by class method functions.

Agreed.  This is redundant; we could safely lose it.

> or
> 
>   // Private method functions facilitate implementation of
>   // the class constructor.
> 
> I consider as completely unnecessary, since it is basic C++ knowledge
> how data in the `private' section works.

In this case, the intent is to emphasize that there is no user, besides
the constructor itself, of any of the private functions; that isn't
normal C++ practice, and is therefore worthy of comment, IMO.

> Just imagine that you are going to insert similarly verbose comments
> for the remaining groff source code! You would have to insert exactly
> the same comments again and again.

Sure.  Excessive redundancy isn't a great idea, but arguably, the
current lack of adequate comments is considerably worse; the code I've
just refactored was, for example, painful to interpret.

> A last thing regarding your comment style: Given that today's editors
> support various colors for comments, data types, etc., I think it's
> better to change
> 
>     // CRLF handling hook, for get_line() function.
>     //
>     int lastc;
> 
>     // Private method functions facilitate implementation of
>     // the class constructor.
>     //
>     int get_line(int);
> 
> to
> 
>     // CRLF handling hook, for get_line() function.
>     int lastc;
> 
>     // Private method functions facilitate implementation of
>     // the class constructor.
>     int get_line(int);

This is a matter of personal preference, but I respectfully disagree:

- we don't always read code in a syntax highlighting text editor; you
don't see it in `less', for example.

- even with syntax highlighting turned on, I find your style to be more
difficult to read, simply because the boundary between comment and code
is obscured when the vertical spacing is omitted.

> Additionally, this would be in sync with other comments in groff.

I doubt that anyone would argue that there isn't room for improvement,
elsewhere. :-)  If we always say "don't bother, because it isn't done
elsewhere", then we never realize any improvement, and it remains poor
forever.  OTOH: "Slowly, slowly, catchee monkey".

> On the other hand, the most important comment is missing: People who
> are having a quick look at the `.psbb' request implementation will
> find the function `ps_bbox_request' first, and there you can see this line:
> 
>     psbb_locator do_ps_file(nm.contents());
> 
> which is very different to the rest of the groff code: It defines
> `do_ps_file' without using it further!  To understand that, you have
> to look up your implementation...  So please change this to
> 
>     // Declaring this class has the intended side effect of setting
>     // `{llx,lly,urx,urx}_reg_contents'.
>     psbb_locator do_ps_file(nm.contents());

Agreed ... and done, (with slightly modified wording -- see attached).

>>> 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).
> 
> Well, `register' is deprecated in the C++11 standard.

Well, groff isn't C++11, but agreed: `register' probably never had the
effect in C++, that it has in C, (where optimization may render it less
valuable today, than it once was, but it still does have an effect).

-- 
Regards,
Keith.

ps_bbox_request.patch
Description: Text Data