>> 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.
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. 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.
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);
Additionally, this would be in sync with other comments in groff.
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());
>> 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.
http://en.cppreference.com/w/cpp/language/storage_duration
You might read
https://gcc.gnu.org/ml/gcc/2010-05/msg00098.html
for more information.
Werner
