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.
diff --git a/src/roff/troff/input.cpp b/src/roff/troff/input.cpp
--- a/src/roff/troff/input.cpp
+++ b/src/roff/troff/input.cpp
@@ -6446,18 +6446,34 @@ inline int psbb_locator::skip_to_trailer
//
// Handle the .psbb request.
//
void ps_bbox_request()
{
+ // Parse input line, to extract file name.
+ //
symbol nm = get_long_name(1);
if (nm.is_null())
+ //
+ // No file name specified: ignore the entire request.
+ //
skip_line();
else {
+ // File name acquired: swallow the rest of the line.
+ //
while (!tok.newline() && !tok.eof())
tok.next();
errno = 0;
+
+ // Update {llx,lly,urx,ury}_reg_contents:
+ // declaring this class instance achieves this, as an
+ // intentional side effect of object construction.
+ //
psbb_locator do_ps_file(nm.contents());
+
+ // All done for .psbb; move on, to continue
+ // input stream processing.
+ //
tok.next();
}
}
const char *asciify(int c)
