bugGNU roff - Bugs: bug #59442, [PATCH] groff.cpp: move soelim...

 
 

bug #59442: [PATCH] groff.cpp: move soelim before preconv in constructed pipeline

Submitter:  Bjarni Ingi Gislason <bjarniig>
Submitted:  Tue 10 Nov 2020 05:26:03 PM UTC
   
 
Category:  General Severity:  1 - Wish
Item Group:  Feature change Status:  Need Info
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Open Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Tue 29 Aug 2023 03:46:43 AM UTC, comment #3: 

comment #2:

> this may indeed be worth considering, but after release, not before.


That time has arrived!

The patch as posted still applies fine (even despite the "verbatim" tags being malformed, which can sometimes cause problems), but the original submission makes clear that the patch is incomplete as-as, pointing out:

> the documentation must then mention this fact in the
> explanation of the "-s" option.

Dave <barx>
Group Member
Wed 11 Nov 2020 11:38:28 AM UTC, comment #2: 

Quick answer without doing a full analysis: this may indeed be worth considering, but after release, not before.  This is not a bugfix but a fundamental change in behaviour, and as such, it could in principle cause serious regressions in existing documents.

Moving to Item Group "Preprocessor preconv" und Category "New feature" because in a nutshell, the idea here is to apply preconv to additional content.

Ingo Schwarze <schwarze>
Group Member
Wed 11 Nov 2020 07:37:27 AM UTC, comment #1: 

Bjari's point seems sound to me.  The whole point of soelim is to get preprocessors to run on `.so`ed files (`.mso`, `.pso`).

I'm trying to think of cases where this would break things where they aren't already broken (we have no test cases here, of course...yet).

Questions to ponder, regarding both current behavior and what happens if we accept the proposed patch.

1. What happens if a .so(urced) file has a non-ASCII character in its filename?  soelim(1) does not seem to speak to this question. Maybe because it is assuming preconv(1) has already been run, or, more likely I think, the issue was not considered when the page was written, since it long predates preconv itself.

2. What about EBCDIC hosts?  Even plain ASCII code points are complete disarranged in EBCDIC.  soelim uses standard C library functions for character comparison and handling (see src/preproc/soelim/soelim.cpp:do_file()).  There are no EBCDIC worries today because preconv has already busted the input down to 7-bit chars.  What about after this patch?  We can't assume that's been done, so chars with the 8th bit set might be in the input.  But I'm not seeing any problems; anything that soelim doesn't care about as documented in its man page, it passes through without alteration, amounting to `putchar(getc());`.  And the chars that soelim DOES care about should not be spuriously matched in a UTF-8 sequence, because all continuation bytes in a UTF-8 sequence have the high bit set.  soelim will, therefore, not spurious match them and munge them.

Can anyone else think of any objections?

G. Branden Robinson <gbranden>
Group administrator
Tue 10 Nov 2020 05:26:03 PM UTC, original submission:  

src/roff/groff/groff.cpp: correct the order of preprocessors in the
pipeline.

  The "soelim" command must be the first in the pipeline to incorporate
 all sourced files into the input file,
 before it is sent to the next command.

  Change the order from "preconv, soelim, refer" to
"soelim, preconv, refer".

  As "preconv" can't (currently) figure out the encoding,
 if it gets its input from standard input,
 "groff" must be called with the option "-K <encoding>" or
 "-D <encoding>", and not just with "-k",
 if the encoding is not the default one.

  This means that the documentation must then mention this fact in the
explanation of the "-s" option.
.verbatim+
Signed-off-by: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
---
 src/roff/groff/groff.cpp | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/roff/groff/groff.cpp b/src/roff/groff/groff.cpp
index f568ac02..cd61dbd0 100644
--- a/src/roff/groff/groff.cpp
+++ b/src/roff/groff/groff.cpp
@@ -53,9 +53,11 @@ extern "C" {
 
 // grap, chem, and ideal must come before pic;
 // tbl must come before eqn
-const int PRECONV_INDEX = 0;
-const int SOELIM_INDEX = PRECONV_INDEX + 1;
-const int REFER_INDEX = SOELIM_INDEX + 1;
+// soelim must come first to complete the input file for the later
+// commands in the pipeline
+const int SOELIM_INDEX = 0;
+const int PRECONV_INDEX = SOELIM_INDEX +1;
+const int REFER_INDEX = PRECONV_INDEX + 1;
 const int GRAP_INDEX = REFER_INDEX + 1;
 const int CHEM_INDEX = GRAP_INDEX + 1;
 const int IDEAL_INDEX = CHEM_INDEX + 1;
--
2.28.0

-verbatim-

Bjarni Ingi Gislason <bjarniig>

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

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 schwarze (Posted a comment)
  • -email is unavailable- added by gbranden (Posted a comment)
  • -email is unavailable- added by bjarniig (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 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-06-10 gbranden CategoryPreprocessor preconv General
        Severity3 - Normal 1 - Wish
    2020-11-11 schwarze CategoryCore Preprocessor preconv
        Item GroupIncorrect behaviour Feature change
    2020-11-11 gbranden Summary[PATCH] groff.cpp: correct the order of preprocessors in the pipeline [PATCH] groff.cpp: move soelim before preconv in constructed pipeline
    2020-11-11 gbranden Item GroupFeature change Incorrect behaviour
        StatusNone Need Info
        Assigned toNone gbranden
    2020-11-10 barx Carbon-CopyRemoved 93119 -
    2020-11-10 barx Summarygroff.cpp: correct the order of preprocessors in the pipeline [PATCH] groff.cpp: correct the order of preprocessors in the pipeline

    Back to the top

    Powered by Savane 3.12