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

[G. Branden Robinson]

Update of bug #61607 (project groff):

                  Status:                    None => Invalid                
             Assigned to:                    None => gbranden               
             Open/Closed:                    Open => Closed                 

    _______________________________________________________

Follow-up Comment #1:

[comment #0 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"


    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?61607>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/