bugGNU roff - Bugs: bug #61433, src/roff/troff/div.h: has extra...

 
 

bug #61433: src/roff/troff/div.h: has extra declaration of "do_divert(...)"

Submitter:  Bjarni Ingi Gislason <bjarniig>
Submitted:  Sat 06 Nov 2021 02:54:41 AM UTC
   
 
Category:  Core Severity:  2 - Minor
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
   

Jump to the original submission

Sun 05 Feb 2023 08:48:50 AM UTC, comment #8: 

Removing patch annotation; we ended up dropping the code, not commenting it out.

G. Branden Robinson <gbranden>
Group administrator
Wed 01 Dec 2021 06:09:10 PM UTC, comment #7: 


commit 74b60f260c597732bd0b58f72a6639157a96132e
Author: G. Branden Robinson <g.branden.robinson@gmail.com>
Date:   Sat Nov 27 23:20:35 2021 +1100

    [troff]: Drop redundant `do_divert()` declaration.

    * src/roff/troff/env.h: Do it.

    Fixes <https://savannah.gnu.org/bugs/?61433>.  Thanks to Bjarni Ingi
    Gislason for the report.

    Also update editor aid comments; drop old style Emacs file-local
    variable setting.


G. Branden Robinson <gbranden>
Group administrator
Wed 01 Dec 2021 12:38:49 AM UTC, comment #6: 

The top of env.h has the comment

// declarations to avoid friend name injection problems

followed by a lengthy list of prototypes, including do_divert().  All of these functions are later listed again when they're made friends of class environment.  If do_divert() is omitted from the first list but retained in the second (as proposed in comment #4), does it warrant a comment to explain the discrepancy to future readers?

Dave <barx>
Group Member
Wed 01 Dec 2021 12:27:11 AM UTC, comment #5: 

Yeah, this is clerical work to keep the compiler happy, so I'd imagine if it compiles without error, everything should work.

Does a GNU program need to support C++ compilers other than gcc?  If so, who has access to one to test it?  I only have gcc.

Dave <barx>
Group Member
Tue 30 Nov 2021 07:52:44 AM UTC, comment #4: 

Oh, no--groff's header files mostly don't use include guards.

<shuts eyes, pinches bridge of nose>

The commit I have queued does not appear to break anything (documents render well, all 94 tests pass or xfail).  I'm likely to just push it as-is.  Here's the relevant bit.


diff --git a/src/roff/troff/env.h b/src/roff/troff/env.h
index 0c8b8b896..dddf1a17d 100644
--- a/src/roff/troff/env.h
+++ b/src/roff/troff/env.h
@@ -140,8 +139,6 @@ void title();
 void widow_control_request();
 #endif /* WIDOW_CONTROL */

-void do_divert(int append, int boxing);
-
 class environment {
   int dummy;                   // dummy environment used for \w
   hunits prev_line_length;





G. Branden Robinson <gbranden>
Group administrator
Tue 30 Nov 2021 07:48:31 AM UTC, comment #3: 


comment #2:

> original submission:
> > except "mstm.cpp" which only includes "env.h".
>
> Bjarni presumably means mtsm.cpp, which does indeed #include env.h but not div.h.  This file doesn't call do_divert(), though, so doesn't need its prototype.
>
> env.h itself, though, later in the file, does refer again to do_divert(): it's made a friend of "class environment".


Thanks, Dave.  I would then assume that env.h should itself #include "div.h", but I'm thinking like a simple C programmer here.  I don't feel I have mastery of C++ name resolution rules, which are much more complex than those of C.

> This is seemingly what prompted Werner to add its prototype (and numerous others) in commit a59ef1e0 to pacify "modern" (as of 2005) C++ compilers.


Yes, that's how I interpret the commit message too.  Thanks for locating this.

> If now-modern compilers don't need this, perhaps the other prototypes that commit added could also go away.  The commit message does hedge by calling these additions "not really necessary."


It looks like far too much work to experimentally revert the changes in that commit one by one.  I'm content with pacifying today's compilers, and with doing what makes sense to my simple C brain unless someone with greater expertise can explain to me how that's counterproductive in this situation.

G. Branden Robinson <gbranden>
Group administrator
Mon 29 Nov 2021 05:31:52 AM UTC, comment #2: 

original submission:

> except "mstm.cpp" which only includes "env.h".


Bjarni presumably means mtsm.cpp, which does indeed #include env.h but not div.h.  This file doesn't call do_divert(), though, so doesn't need its prototype.

env.h itself, though, later in the file, does refer again to do_divert(): it's made a friend of "class environment".

This is seemingly what prompted Werner to add its prototype (and numerous others) in commit a59ef1e0 to pacify "modern" (as of 2005) C++ compilers.  If now-modern compilers don't need this, perhaps the other prototypes that commit added could also go away.  The commit message does hedge by calling these additions "not really necessary."

Dave <barx>
Group Member
Sat 27 Nov 2021 12:14:22 PM UTC, comment #1: 

Thanks for the report.  I think a better fix for this is not to comment out the declaration in div.h, but to remove the redundant declaration in env.h.

G. Branden Robinson <gbranden>
Group administrator
Sat 06 Nov 2021 02:54:41 AM UTC, original submission:  

From a9b2ffe678c6a5ccf3fcae185ace9483d53f1c83 Mon Sep 17 00:00:00 2001
From: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
Date: Sat, 6 Nov 2021 02:45:35 +0000
Subject: [PATCH] src/roff/troff/div.h: has extra declaration of
 "do_divert(...)"

  Both "div.h" and "env.h" include "do_divert(...)".

  cpp-files include either both "env.h" and "div.h" or neither,
except "mstm.cpp" which only includes "env.h".

  gcc 11.2.0-10 (Debian testing) reports a repeated declaration of
"do_divert(...)" so it should be commented out in "div.h".

In file included from ../src/roff/troff/div.cpp:31:
../src/roff/troff/div.h:20:6: warning: redundant redeclaration of 'void do_divert(int, int)' in same scope [-Wredundant-decls]
   20 | void do_divert(int append, int boxing);
      |      ^~~~~~~~~
In file included from ../src/roff/troff/div.cpp:27:
../src/roff/troff/env.h:401:15: note: previous declaration of 'void do_divert(int, int)'
  401 |   friend void do_divert(int append, int boxing);
      |               ^~~~~~~~~
  CXX      src/roff/troff/env.o
In file included from ../src/roff/troff/env.cpp:28:
../src/roff/troff/div.h:20:6: warning: redundant redeclaration of 'void do_divert(int, int)' in same scope [-Wredundant-decls]
   20 | void do_divert(int append, int boxing);
      |      ^~~~~~~~~
In file included from ../src/roff/troff/env.cpp:24:
../src/roff/troff/env.h:401:15: note: previous declaration of 'void do_divert(int, int)'
  401 |   friend void do_divert(int append, int boxing);
      |               ^~~~~~~~~
  CXX      src/roff/troff/input.o
In file included from ../src/roff/troff/input.cpp:30:
../src/roff/troff/div.h:20:6: warning: redundant redeclaration of 'void do_divert(int, int)' in same scope [-Wredundant-decls]
   20 | void do_divert(int append, int boxing);
      |      ^~~~~~~~~
In file included from ../src/roff/troff/input.cpp:26:
../src/roff/troff/env.h:401:15: note: previous declaration of 'void do_divert(int, int)'
  401 |   friend void do_divert(int append, int boxing);
      |               ^~~~~~~~~
  CXX      src/roff/troff/mtsm.o
  CXX      src/roff/troff/node.o
In file included from ../src/roff/troff/node.cpp:35:
../src/roff/troff/div.h:20:6: warning: redundant redeclaration of 'void do_divert(int, int)' in same scope [-Wredundant-decls]
   20 | void do_divert(int append, int boxing);
      |      ^~~~~~~~~
In file included from ../src/roff/troff/node.cpp:31:
../src/roff/troff/env.h:401:15: note: previous declaration of 'void do_divert(int, int)'
  401 |   friend void do_divert(int append, int boxing);
      |               ^~~~~~~~~
../src/roff/troff/node.cpp: In copy constructor 'tfont_spec::tfont_spec(const tfont_spec&)':
../src/roff/troff/node.cpp:192:48: warning: implicitly-declared 'constexpr tfont_spec& tfont_spec::operator=(const tfont_spec&)' is deprecated [-Wdeprecated-copy]
  192 |   tfont_spec(const tfont_spec &spec) { *this = spec; }
      |                                                ^~~~
../src/roff/troff/node.cpp:192:3: note: because 'tfont_spec' has user-provided 'tfont_spec::tfont_spec(const tfont_spec&)'
  192 |   tfont_spec(const tfont_spec &spec) { *this = spec; }
      |   ^~~~~~~~~~
  CXX      src/roff/troff/number.o
In file included from ../src/roff/troff/number.cpp:26:
../src/roff/troff/div.h:20:6: warning: redundant redeclaration of 'void do_divert(int, int)' in same scope [-Wredundant-decls]
   20 | void do_divert(int append, int boxing);
      |      ^~~~~~~~~
In file included from ../src/roff/troff/number.cpp:24:
../src/roff/troff/env.h:401:15: note: previous declaration of 'void do_divert(int, int)'
  401 |   friend void do_divert(int append, int boxing);
      |               ^~~~~~~~~

Signed-off-by: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
---
 src/roff/troff/div.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

  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 #52210:  0001-src-roff-troff-div.h-has-extra-declaration-of-do_diver.txt added by bjarniig (4KiB - text/plain - Project groff, comment out a declaration in div.h)

 

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 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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-02-05 gbranden Summary[PATCH] src/roff/troff/div.h: has extra declaration of &quot;do_divert(...)&quot; src/roff/troff/div.h: has extra declaration of "do_divert(...)"
    2021-12-01 gbranden StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Planned ReleaseNone 1.23.0
    2021-11-27 gbranden Severity3 - Normal 2 - Minor
        StatusNone In Progress
        Assigned toNone gbranden
    2021-11-06 bjarniig Attached File- Added 0001-src-roff-troff-div.h-has-extra-declaration-of-do_diver.txt, #52210

    Back to the top

    Powered by Savane 3.15-e6e5.
    Corresponding source code