bugGNU roff - Bugs: bug #60913, suboptimal diagnostics and...

 
 

bug #60913: suboptimal diagnostics and spurious output when (mis)using .evc

Submitter:  G. Branden Robinson <gbranden>
Submitted:  Mon 12 Jul 2021 06:26:11 PM UTC
   
 
Category:  Core Severity:  3 - Normal
Item Group:  Warning/Suspicious behaviour Status:  Fixed
Privacy:  Public Assigned to:  gbranden
Open/Closed:  Closed Planned Release:  1.23.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Thu 15 Jul 2021 10:31:55 PM UTC, comment #3: 


commit ff394f136b602454c557d0ce96cd788db36d138b
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Tue Jul 13 06:11:07 2021 +1000

    [troff]: Refactor environment handling.

    [troff]: Refactor environment initialization, switching, and copying.

    * src/roff/troff/env.cpp: Rename struct `env_list` to `env_list_node`
      since it describes a node of a singly-linked list.  Remove constant
      `NENVIRONMENTS` and array `env_table`.  Add static symbol
      `default_environment_name` to replace string literal.

      (init_environments): Stop initializing `curenv` through `env_table`.
      Use `default_environment_name` for that initialization and add the
      default environment to `env_dictionary`.

      (environment_switch): Simplify.  Shorten "dummy environment"
      diagnostic message.  Stop creating an integer-named environment inside
      the `env_table` array, only falling through to use the
      `env_dictionary` if the named environment is not a valid integer or if
      the array is full.  Instead use `env_dictionary` always.  Drop no
      longer needed `pop` quasi-Boolean integer with extra state to suppress
      environment stack underflow errors.  Instead report the error if
      underflow occurs, regardless of any other circumstance.

      (environment_copy): Simplify.  Stop searching the `env_table` array
      for an environment to copy from, only falling through to use the
      `env_dictionary` if the named environment is not a valid integer or if
      the array is full.  Instead search `env_dictionary` always.  Emit "no
      environment specified to copy from" diagnostic only if the `evc`
      request is given no argument.  If the source environment to copy from
      is given but not found, emit a new diagnostic naming the nonexistent
      environment.  Fix bug: stop returning early if no copying could be
      done; instead fall through to the end of the function, which calls
      `skip_line()` and prevents anything on the remainder of the (invalid)
      control line from being interpreted.  Problem dates back to commit
      da3b7137, 6 March 2000 (groff 1.16).

    Fixes <https://savannah.gnu.org/bugs/?60913>.

commit 9d86a8e3f68644e891878bbda12497633f1b1bb5
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Tue Jul 13 06:29:31 2021 +1000

    Regression-test Savannah #60913.

    Warning: test fails at this commit.


G. Branden Robinson <gbranden>
Group administrator
Mon 12 Jul 2021 07:01:13 PM UTC, comment #2: 


comment #1:

> Also, when .ev and .evc are misused, they cause empty lines in the output because the validation logic performs early returns without calling skip_line() to discard the newline at the end of the control line (and any other stuff before it).


Correction: only .evc misbehaves in this way.

Alas--now I only get to claim the head of a 21 year-old bug instead of a 30 year-old one.

G. Branden Robinson <gbranden>
Group administrator
Mon 12 Jul 2021 06:42:51 PM UTC, comment #1: 

Also, when .ev and .evc are misused, they cause empty lines in the output because the validation logic performs early returns without calling skip_line() to discard the newline at the end of the control line (and any other stuff before it).


$ cat EXPERIMENTS/envbad.groff
.ev \" environment stack underflow
.evc nonexistent \" indeed it doesn't exist
.pl \n[nl]u
$ groff -Tutf8 -E EXPERIMENTS/envbad.groff | wc
      1       0       1
$ ./build/test-groff -Tutf8 -E EXPERIMENTS/envbad.groff | wc
      0       0       0


G. Branden Robinson <gbranden>
Group administrator
Mon 12 Jul 2021 06:26:11 PM UTC, original submission:  

.evc gives the same diagnostic for a missing argument as for an environment name that doesn't exist.

Also, when warnings of type `missing` are enabled, multiple diagnostics are emitted for the same problem.

Here's output from groff 1.22.4 and Git HEAD.


$ nroff -ww | cat -s
.evc
troff: <standard input>:1: warning: missing number
troff: <standard input>:1: No environment to copy from
.evc nope
troff: <standard input>:2: No environment to copy from


We can do better than this.  "[M]issing number" is downright misleading.

I have a patch in progress to resolve this and also perform a simplifying refactor of the environment switching and copying implementations.  For some reason both a 10-element array and a dynamically-managed dictionary of environments were managed.  However, as far as I can tell, the only thing that was ever stored in the environment array (`env_table`) was the default environment named "0".  I'm guessing the array stuff is a relic of an early implementation effort that was never completely cleaned up.


 env.cpp |   99 ++++++++++++++++++----------------------------------------------
 1 file changed, 28 insertions(+), 71 deletions(-)


G. Branden Robinson <gbranden>
Group administrator

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #51663:  env.diff added by gbranden (9KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gbranden (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-07-15 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2021-07-12 gbranden Severity2 - Minor 3 - Normal
        Summarysuboptimal diagnostics when (mis)using .evc suboptimal diagnostics and spurious output when (mis)using .evc
    2021-07-12 gbranden Attached File- Added env.diff, #51663

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code