bugGNU roff - Bugs: bug #61607, tbl/table.cpp: avoid creating a...

 
 

bug #61607: tbl/table.cpp: avoid creating a trailing space when the last column is centered

Submitter:  Bjarni Ingi Gislason <bjarniig>
Submitted:  Fri 03 Dec 2021 01:32:43 AM UTC
   
 
Category:  Preprocessor tbl Severity:  3 - Normal
Item Group:  Feature change Status:  Invalid
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Fri 03 Dec 2021 07:59:05 PM UTC, comment #2: 

Sorry, one error and one possibly misleading statement in my comment.

comment #1:

> (I'm not sure, because "extern tricks" have been discouraged in C for longer than I've been programming in the language--see Klemens, 21st Century C, 2nd edition, O'Reilly 2015, p. 174--but it is also possible that by doing the foregoing you have opened an interference channel with symbols of the same name in completely different translation units, even with libraries that the program may be linking with.


I've been programming in C since well before 2015.  I'm old enough to remember being excited by C99 coming out.  I cited the foregoing book because (1) it's excellent and (2) it mentions "crazy linkage tricks across different scopes within one file" as something that is "generally not done".

> The foregoing suggests to me that what you're really trying to do is change the semantics of `nc` (a poorly named member variable).


I misspoke here.  `nc` is not a member variable but an argument to a constructor.  (I still feel it's poorly named.)

G. Branden Robinson <gbranden>
Group administrator
Fri 03 Dec 2021 07:51:52 PM UTC, comment #1: 

original submission:

> From d447f8bb823e9978d453eef38c203d6fa5ed044b Mon Sep 17 00:00:00 2001
> From: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
> Date: Fri, 3 Dec 2021 01:20:35 +0000
> Subject: [PATCH] tbl/table.cpp: avoid creating a trailing space when the last
>  column is centered
>
>   Directory: src/preproc/
>
>   The padding character is "^C" in ".fc ^B^C" ('^'
> means a control character)
>
>   Example:
>
> .TS
> c .
> centered
> .TE
>
> /usr/bin/tbl <file> :
>
> [...]
> .fc ^B^C
> [...]
> .nr 3cl0 0*\n[3sep]
> [...]
> \&\h'|\n[3cl0]u'^B^Ccentered^C^B
>
>   The last padding character is interpreted as a space character(?),


I don't think that it is.

> but is removed in "env.cpp" (strip trailing spaces) without reporting it.


This remark is reminiscent of your preoccupation with this subject in another recent ticket, bug #61605.

>   The fix is to remove the last '^C'.


We can demonstrate that the padding character is not "converted into space" by adding some columns before and after the centered one, populating the corresponding table entries with spaceless data, and forcing the column separation after the first two columns to zero.  If any space results, it must be getting injected by tbl(1).

But I see no such behavior with groff 1.22.4 or Git HEAD.


$ cat EXPERIMENTS/tbl-branden-center.roff
.TS
tab($);
L0 C0 L.
foo$centered$bar
.TE
$ nroff -t EXPERIMENTS/tbl-branden-center.roff | cat -s
foocenteredbar

$ ./build/test-groff -b -ww -T utf8 -t EXPERIMENTS/tbl-branden-center.roff | cat -s
foocenteredbar


No extraneous spaces are in evidence.

>   The patch is in the attachment.


The patch contains irrelevant changes.

Here is a code review.


diff --git a/src/preproc/tbl/table.cpp b/src/preproc/tbl/table.cpp
index 435ac6bf7..364c26f19 100644
--- a/src/preproc/tbl/table.cpp
+++ b/src/preproc/tbl/table.cpp
@@ -546,10 +546,15 @@ center_text_entry::center_text_entry(const table *p,

 void center_text_entry::simple_print(int)
 {
+  extern int center_ncolumns;


This is highly non-idiomatic C, and not something I've seen in my modest exposure to C++, either.

If you want a global, i.e., a variable accessible from other functions, put this variable in file scope (you did--see below). 

(I'm not sure, because "extern tricks" have been discouraged in C for longer than I've been programming in the language--see Klemens, 21st Century C, 2nd edition, O'Reilly 2015, p. 174--but it is also possible that by doing the foregoing you have opened an interference channel with symbols of the same name in completely different translation units, even with libraries that the program may be linking with.  `extern` means "external linkage".  It is thus grossly overpowered for the symbol visibility problem you are trying to solve.)

In C++, if you want to share data between member functions like this, the obvious solution is a member variable of the `center_text_entry` class.


   printfs("\\h'|\\n[%1]u'", column_start_reg(start_col));
   prints("\002\003");
   print_contents();
-  prints("\003\002");
+/* Avoid creating a trailing space in the last column */
+  if (start_col == (center_ncolumns - 1) )
+    prints("\002");
+  else
+    prints("\003\002");


This is the heart of the change.


 }

 void center_text_entry::add_tab()
@@ -1238,6 +1243,9 @@ void vertical_rule::print()
   }
 }

+/* Used in function center_text_entry::simple_print(int) */
+int center_ncolumns;
+


See?  Here's the global.  You shouldn't need the `extern` declaration inside `text_entry::simple_print`.  Unless C++ name resolution rules hide it (I have to admit I haven't mastered those yet).  If your code breaks without the earlier `extern`, then making this object a member variable of the class is the way to go.

If the problem was simply a forward declaration, just move this global before defining `text_entry::simple_print`.


 table::table(int nc, unsigned f, int ls, char dpc)
 : nrows(0), ncolumns(nc), linesize(ls), decimal_point_char(dpc),
   vrule_list(0), stuff_list(0), span_list(0),
@@ -1245,6 +1253,7 @@ table::table(int nc, unsigned f, int ls, char dpc)
   vline(0), row_is_all_lines(0), left_separation(0), right_separation(0),
   total_separation(0), allocated_rows(0), flags(f)
 {
+  center_ncolumns = nc;


The foregoing suggests to me that what you're really trying to do is change the semantics of `nc` (a poorly named member variable).  If I were trying to attack this problem it would give me pause, and cause me to see where else nc is used and for what purposes.  Possibly a new variable is not needed at all.


   minimum_width = new string[ncolumns];
   column_separation = ncolumns > 1 ? new int[ncolumns - 1] : 0;
   equal = new char[ncolumns];
@@ -1913,7 +1922,7 @@ void table::init_output()
            ".nr " SAVED_DN_REG " \\n[dn]\n"
            ".ne \\n[dn]u+\\n[.V]u\n"
            ".ie \\n[.t]<=\\n[" SAVED_DN_REG "] \\{\\\n"
-           ".  tmc \\n[.F]: around line \\n[.c]: error:\n"
+           ".  tmc tbl:\\n[.F]: around line \\n[.c]: error:\n"
            ".  tmc \" table will not fit on page \\n%;\n"
            ".  tm1 \" use .TS H/.TH with a supporting macro package\n"
            ".\\}\n"


This change is completely irrelevant to this ticket, and should not have been included as part of this patch.  We've also discussed this change before.  As I've said, I believe adding "tbl:" makes the diagnostic misleading.  This diagnostic is not being produced by tbl(1), but by your document.  Yes, it is produced by groff output generated by tbl, but that is an expansion of your table definition.  If you try to render a table on a page too small to fit it, the problem is with your data or your page, not with tbl.


@@ -2023,14 +2032,14 @@ string column_end_reg(int col)

 string column_divide_reg(int col)
 {
-  static char name[sizeof(COLUMN_DIVIDE_PREFIX)+INT_DIGITS];
+  static char name[sizeof(COLUMN_DIVIDE_PREFIX)+INT_DIGITS+1]; /* Added 1, BG */
   sprintf(name, COLUMN_DIVIDE_PREFIX "%d", col);
   return string(name);
 }

 string row_top_reg(int row)
 {
-  static char name[sizeof(ROW_TOP_PREFIX)+INT_DIGITS];
+  static char name[sizeof(ROW_TOP_PREFIX)+INT_DIGITS+1]; /* Added 1, BG */


Why are you growing these buffers by 1 character when the heart of your change is to write 1 fewer character in some circumstances?  Do the above 2 changes in fact have anything to do with this bug report at all?

The remaining changes are further alterations to the diagnostic messages, and as ill-advised as the one already discussed, in my opinion.


   sprintf(name, ROW_TOP_PREFIX "%d", row);
   return string(name);
 }
@@ -2177,7 +2186,7 @@ void table::compute_expand_width()
            "delim off\n"
            ".EN\n"
            "..\n");
-    prints(".tmc \\n[.F]: around line \\n[.c]: warning:\n"
+    prints(".tmc tbl:\\n[.F]: around line \\n[.c]: warning:\n"
            ".tm1 \" table wider than line width\n");
     prints(".ig\n"
            ".EQ\n"
@@ -2230,7 +2239,7 @@ void table::compute_separation_factor()
            "delim off\n"
            ".EN\n"
            "..\n");
-    prints(".tmc \\n[.F]: around line \\n[.c]: warning:\n"
+    prints(".tmc tbl:\\n[.F]: around line \\n[.c]: warning:\n"
            ".tm1 \" table column separation set to zero\n"
            ".nr " SEPARATION_FACTOR_REG " 0\n");
   }
@@ -2238,7 +2247,7 @@ void table::compute_separation_factor()
          ".el .if \\n[" SEPARATION_FACTOR_REG "]<1n \\{\\\n");
   entry_list->set_location();
   if (!(flags & NOWARN)) {
-    prints(".tmc \\n[.F]: around line \\n[.c]: warning:\n"
+    prints(".tmc tbl:\\n[.F]: around line \\n[.c]: warning:\n"
            ".tm1 \" table squeezed horizontally to fit line length\n");
     prints(".ig\n"
            ".EQ\n"


G. Branden Robinson <gbranden>
Group administrator
Fri 03 Dec 2021 01:32:43 AM UTC, original submission:  

From d447f8bb823e9978d453eef38c203d6fa5ed044b Mon Sep 17 00:00:00 2001
From: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
Date: Fri, 3 Dec 2021 01:20:35 +0000
Subject: [PATCH] tbl/table.cpp: avoid creating a trailing space when the last
 column is centered

  Directory: src/preproc/

  The padding character is "^C" in ".fc ^B^C" ('^'
means a control character)

  Example:

.TS
c .
centered
.TE

/usr/bin/tbl <file> :

[...]
.fc ^B^C
[...]
.nr 3cl0 0*\n[3sep]
[...]
\&\h'|\n[3cl0]u'^B^Ccentered^C^B

  The last padding character is interpreted as a space character(?),
but is removed in "env.cpp" (strip trailing spaces) without reporting
it.

  The fix is to remove the last '^C'.

Signed-off-by: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
---
 src/preproc/tbl/table.cpp | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

  The patch is in the attachment.

Bjarni Ingi Gislason <bjarniig>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #52418:  0001-tbl-table.cpp-avoid-creating-a-trailing-space-when-the.txt added by bjarniig (4KiB - text/plain - patch for tbl/table.cpp project groff)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -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 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-06-10 gbranden Summary [PATCH] tbl/table.cpp: avoid creating a trailing space when the last column is centered tbl/table.cpp: avoid creating a trailing space when the last column is centered
    2021-12-03 gbranden StatusNone Invalid
        Assigned toNone gbranden
        Open/ClosedOpen Closed
    2021-12-03 barx Carbon-CopyRemoved 93119 -
    2021-12-03 barx Summary [[PATCH] tbl/table.cpp: avoid creating a trailing space when the last column is centered [PATCH] tbl/table.cpp: avoid creating a trailing space when the last column is centered
    2021-12-03 bjarniig Attached File- Added 0001-tbl-table.cpp-avoid-creating-a-trailing-space-when-the.txt, #52418

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code