bugGNU roff - Bugs: bug #59817, src/roff/troff/input.cpp: flatten...

 
 

bug #59817: src/roff/troff/input.cpp: flatten .ec parser

Submitter:  ivan tkachenko <ratijas>
Submitted:  Mon 04 Jan 2021 04:03:52 PM UTC
   
 
Category:  Core Severity:  3 - Normal
Item Group:  Lint Status:  Rejected
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 30 Mar 2023 05:24:01 PM UTC, comment #6: 

Dropping patch annotation from rejected ticket.

G. Branden Robinson <gbranden>
Group administrator
Thu 17 Jun 2021 10:08:50 AM UTC, comment #5: 

There has been no follow-up after almost 3 months; closing.

G. Branden Robinson <gbranden>
Group administrator
Wed 31 Mar 2021 03:49:38 AM UTC, comment #4: 

comment #3:

> comment #2:
> > A comment explicitly tying this function to the user-level .ec
> > request would be illuminating to someone coming to this snippet cold.
>
> I may have to retract this.  Looking at the rest of the file, it seems init_input_requests() rather clearly associates roff-language requests with input.cpp functions.


Agreed.  I have frequently visited that function in my travels through the source. :D

Again I'll say, "[m]ore helpful I think would simply be a comment above the function giving it a specification."


G. Branden Robinson <gbranden>
Group administrator
Wed 31 Mar 2021 03:13:50 AM UTC, comment #3: 

comment #2:

> A comment explicitly tying this function to the user-level .ec
> request would be illuminating to someone coming to this snippet cold.


I may have to retract this.  Looking at the rest of the file, it seems init_input_requests() rather clearly associates roff-language requests with input.cpp functions.

Dave <barx>
Group Member
Mon 29 Mar 2021 07:15:17 AM UTC, comment #2: 

I have no opinion on the actual code of the original vs the patch, though I share Branden's doubt that such micro-optimizations give any measurable benefit to execution speed.

Concerning code comments:

comment #1:

> original submission:
> > Comments (copied from groff(7)) are supposed to help travelers navigate
> > their way through the code base.
>
> More helpful I think would simply be a comment above the
> function giving it a specification.  Individual lines of code
> should require commenting only if they're having to work
> around a bug elsewhere, or if they're doing something "clever"


I agree with Branden here: the logic is easy enough to follow that individual branches of the conditionals don't need their own comments, but that what's missing is an overview of what set_escape_char() does.  A comment explicitly tying this function to the user-level .ec request would be illuminating to someone coming to this snippet cold.  The logic then becomes fairly self-explanatory.

The other patch attached here (file #50642) seems uncontroversial and worth applying.

Dave <barx>
Group Member
Tue 05 Jan 2021 02:03:39 AM UTC, comment #1: 

Hi Ivan!

original submission:

> Using local temporary `int c` contributes to readability


I'm afraid I don't agree.  It's possible I've spent enough time looking at input.cpp that I've grown accustomed to James Clark's idioms (and the GNU coding style :-O ), but I don't find the logic of this short function difficult to follow.

> and saves from some extra assignments.
> Might have improved code generation as well.


GNU troff is not a high-performance application.  As you may know, for many years it did not even have a test suite, and even now it has only a handful of tests that directly exercise certain features (we have some fairly effective de facto integration tests thanks to some complex documents that are rendered as part of the build process, on the other hand).

I'm more concerned with verifying the correctness of the formatter's operation than optimizing its performance at this point.  Knuth's famous maxim about premature optimization applies here, I think.

Furthermore, performance claims should always be quantified, or at least concretized.  If a person is unwilling or unable to show the better assembly language that is generated by a patch, I am reflexively skeptical about claims of improved code generation.  (Doing so opens the can of worms that is the variable impact of such changed for different instruction set architectures, which I suspect is one reason people prefer to hand-wave this issue.  On the gripping hand, I wouldn't be surprised to learn that for pure user-mode code like this, almost every ISA in use by POSIX systems is comparable after adjusting for implementation issues like instruction width, clock speed, number of cores, pipelining, and so forth.)

If this were C, you'd have an easier time convincing me, because you're right that tok.ch() appears twice, and as a general rule I try to minimize function calls to obtain a value when that value is not expected to change.  In C, saving the value to a temporary makes sense, because a thing that looks like a function call usually is one, and function calls are inherently more expensive than basic blocks.  In C++, compilers have to be "smarter" and there's a deeper idiom of getters and setters.  C++ is a higher-level language than C, and one of the things we trade away for higher-level abstractions is performance, despite much emphasis given to "zero-cost abstractions" by C++ advocates.

In the instant case, there may be no function call at all because token.ch() is defined as an inline function (see src/roff/troff/token.h).

> Comments (copied from groff(7)) are supposed to help travelers navigate
> their way through the code base.


More helpful I think would simply be a comment above the function giving it a specification.  Individual lines of code should require commenting only if they're having to work around a bug elsewhere, or if they're doing something "clever", for which a programmer usually owes the reader an apology.

> For future contributions, what's the right way to send patches here? Can I use git send-email? How?


I don't know of a way to mail attachments to Savannah, but I know little of Savannah's ticket tracking system.  `git --format-patch` with appropriate arguments seems to work fine for our contributors, and can be reliably used with `git am`.  It looks like that's what you already did here, so that's fine.

Here's the meat of your patch for web/email reader convenience.


diff --git a/src/roff/troff/input.cpp b/src/roff/troff/input.cpp
index 742447f6..16fa69c4 100644
--- a/src/roff/troff/input.cpp
+++ b/src/roff/troff/input.cpp
@@ -168,16 +168,18 @@ void chop_macro();                // declare to avoid friend name injection

 void set_escape_char()
 {
-  if (has_arg()) {
-    if (tok.ch() == 0) {
-      error("bad escape character");
-      escape_char = '\\';
-    }
-    else
-      escape_char = tok.ch();
+  int c = '\\';
+  if (!has_arg()) {
+    // .ec        Reset escape character to '\'.
+  } else if (tok.ch() == 0) {
+    error("bad escape character");
+    // Also reset to '\'.
+  } else {
+    // .ec c    Set escape character to c.
+    c = tok.ch();
   }
-  else
-    escape_char = '\\';
+
+  escape_char = c;
   skip_line();
 }


Thanks for the report.  While I'm leaning against merging it, I am open to rebuttals, either yours or those of others.

Regards,
Branden

G. Branden Robinson <gbranden>
Group administrator
Mon 04 Jan 2021 04:03:52 PM UTC, original submission:  

Using local temporary `int c` contributes to readability and saves from
some extra assignments. Might have improved code generation as well.

Comments (copied from groff(7)) are supposed to help travelers navigate
their way through the code base.

---

For future contributions, what's the right way to send patches here? Can I use git send-email? How?

ivan tkachenko <ratijas>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by barx (Posted a comment)
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by ratijas (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-03-30 gbranden Summary[PATCH] src/roff/troff/input.cpp: Flatten .ec parser src/roff/troff/input.cpp: flatten .ec parser
    2022-01-22 gbranden Item GroupNone Lint
        StatusInvalid Rejected
    2021-06-17 gbranden StatusNeed Info Invalid
        Open/ClosedOpen Closed
    2021-01-05 gbranden StatusNone Need Info
        Assigned toNone gbranden
    2021-01-04 ratijas Attached File- Added 0001-src-roff-troff-input.cpp-Remove-trailing-whitespaces.patch, #50642
        Attached File- Added 0002-src-roff-troff-input.cpp-Flatten-.ec-parser.patch, #50643

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code