bugGNU roff - Bugs: bug #58314, preconv.cpp: Add block delimiters...

 
 

bug #58314: preconv.cpp: Add block delimiters for an if-clause

Submitter:  Bjarni Ingi Gislason <bjarniig>
Submitted:  Wed 06 May 2020 08:39:34 PM UTC
   
 
Category:  Preprocessor preconv Severity:  1 - Wish
Item Group:  Warning/Suspicious behaviour Status:  Invalid
Privacy:  Public Assigned to:  schwarze
Open/Closed:  Closed Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Fri 08 May 2020 12:30:50 PM UTC, comment #2: 

This ticket is now moot because gbranden@ decided to revert the code change causing the compiler warning, for reasons unrelated to this ticket.

Anyway, i consider opening tickets for personal style issues that  clearly do not indicate bugs rather pointless behaviour.

> ../src/preproc/preconv/preconv.cpp:959:6: warning:
> suggest explicit braces to avoid ambiguous 'else' [-Wdangling-else]
>   959 |   if (fseek(fp, -limit, SEEK_END) != 0)


Most coding styles agree that

        if (outer) {
                if (inner)
                        do_this();
                else
                        do_that();
        }

is easier to read than

        if (outer) 
                if (inner)
                        do_this();
                else
                        do_that();

which you used.


gbranden@ wrote:

> https://www.gnu.org/prep/standards/html_node/Formatting.html


I don't see that page talking about -Wdangling-else in particular.

> That's not the brace style in use in the codebase.


It is, for example look at the functions unicode_entity() and conversion_iconv() in the file preconv.cpp which contain if-else statements right inside if and else clauses and which do use braces on the outer if and else clauses even though these outer braces are not required by the C standard.

> My own brace style preference is 1TBS.
> I welcome comments from other groff developers.


Unsurprisingly, my personal preference is K&R style in the BSD KNF variant in the OpenBSD KNF variant,

  https://man.openbsd.org/style.9

but i don't think that matters here at all.  :-)

Ingo Schwarze <schwarze>
Group Member
Thu 07 May 2020 10:25:56 PM UTC, comment #1: 

Please remember the '-verbatim-' tag in the future.  The Savannah bug renderer on the Web is not very robust in its absence.

original submission:

> From bfda94f0ffbf1cce3a35370f650564affe4a5535 Mon Sep 17 00:00:00 2001
> From: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
> Date: Wed, 6 May 2020 20:29:43 +0000
> Subject: [PATCH] preconv.cpp: Add block delimiters for an if-clause
>
>   Warning from the compiler:


> ../src/preproc/preconv/preconv.cpp: In function 'char* get_late_coding_tag(FILE*)':
> ../src/preproc/preconv/preconv.cpp:959:6: warning: suggest explicit braces to avoid ambiguous 'else' [-Wdangling-else]
>   959 |   if (fseek(fp, -limit, SEEK_END) != 0)
>       |      ^
>
>   Always use block delimiters to help the compiler to know explicitly
> what belongs to a block.

That's not the brace style in use in the codebase.

>   Indention is a pseudo (unreal) structure element, intended only for
> humans.
>
>   Compilers don't "see" nor do they count, how many tabs or space
> characters there are in front of a command word.

I'm aware of all this.  My own brace style preference is 1TBS.

The groff brace style is pretty close to that mandated by the GNU Coding Standards:

https://www.gnu.org/prep/standards/html_node/Formatting.html

However, a grep reveals that the existing codebase does not brace 'do-while' structures as GNU instructs.

Possibly an oversight, as do-whiles are rare relative to while-do.

The code works as I intend (I have a whole pile of negative test cases I can share).  Does this warning get thrown for anything else in the groff source tree?

I welcome comments from other groff developers.

G. Branden Robinson <gbranden>
Group administrator
Wed 06 May 2020 08:39:34 PM UTC, original submission:  

From bfda94f0ffbf1cce3a35370f650564affe4a5535 Mon Sep 17 00:00:00 2001
From: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
Date: Wed, 6 May 2020 20:29:43 +0000
Subject: [PATCH] preconv.cpp: Add block delimiters for an if-clause

  Warning from the compiler:

../src/preproc/preconv/preconv.cpp: In function 'char* get_late_coding_tag(FILE*)':
../src/preproc/preconv/preconv.cpp:959:6: warning: suggest explicit braces to avoid ambiguous 'else' [-Wdangling-else]
  959 |   if (fseek(fp, -limit, SEEK_END) != 0)
      |      ^

  Always use block delimiters to help the compiler to know explicitly
what belongs to a block.

  Indention is a pseudo (unreal) structure element, intended only for
humans.

  Compilers don't "see" nor do they count, how many tabs or space
characters there are in front of a command word.

Signed-off-by: Bjarni Ingi Gislason <bjarniig@rhi.hi.is>
---
 src/preproc/preconv/preconv.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/preproc/preconv/preconv.cpp b/src/preproc/preconv/preconv.cpp
index b4da04d0..f068bad6 100644
--- a/src/preproc/preconv/preconv.cpp
+++ b/src/preproc/preconv/preconv.cpp
@@ -956,11 +956,12 @@ get_late_coding_tag(FILE *fp)
   if (fseek(fp, 0, SEEK_END) != 0)
     return NULL;
   // Seek to `limit` bytes from the end of the buffer, or the beginning.
-  if (fseek(fp, -limit, SEEK_END) != 0)
+  if (fseek(fp, -limit, SEEK_END) != 0) {
     if (errno == EINVAL)
       rewind(fp);
     else
       return NULL;
+  }
   char *tmpbuf = (char *) calloc(1, limit + 1 /* trailing '\0' */);
   if (!tmpbuf) {
     error("unable to allocate memory");
--
2.26.2
-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 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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-06-10 gbranden Assigned togbranden schwarze
        Summary[PATCH] preconv.cpp: Add block delimiters for an if-clause preconv.cpp: Add block delimiters for an if-clause
    2020-05-08 schwarze CategoryCore Preprocessor preconv
        Severity3 - Normal 1 - Wish
        StatusNeed Info Invalid
        Open/ClosedOpen Closed
    2020-05-07 gbranden StatusNone Need Info
        Assigned toNone gbranden

    Back to the top

    Powered by Savane 3.13-4b48.
    Corresponding source code