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

[Bjarni Ingi Gislason]

URL:
<https://savannah.gnu.org/bugs/?58314>

Summary: [PATCH] preconv.cpp: Add block delimiters for an
if-clause
Project: GNU troff
Submitted by: bjarniig
Submitted on: Wed 06 May 2020 08:39:34 PM UTC
Category: Core
Severity: 3 - Normal
Item Group: Warning/Suspicious behaviour
Status: None
Privacy: Public
Assigned to: None
Open/Closed: Open
Discussion Lock: Any
Planned Release: None

_______________________________________________________

From bfda94f0ffbf1cce3a35370f650564affe4a5535 Mon Sep 17 00:00:00 2001

From: Bjarni Ingi Gislason <***@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 <***@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





_______________________________________________________

Reply to this item at:

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

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

[G. Branden Robinson]

Update of bug #58314 (project groff):

Status: None => Need Info
Assigned to: None => gbranden

_______________________________________________________

Follow-up Comment #1:

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

[comment #0 original submission:]

From bfda94f0ffbf1cce3a35370f650564affe4a5535 Mon Sep 17 00:00:00 2001
Date: Wed, 6 May 2020 20:29:43 +0000
Subject: [PATCH] preconv.cpp: Add block delimiters for an if-clause
../src/preproc/preconv/preconv.cpp: In function 'char*
../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.

_______________________________________________________

Reply to this item at:

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

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

[Ingo Schwarze]

Update of bug #58314 (project groff):

Category: Core => Preprocessor preconv
Severity: 3 - Normal => 1 - Wish
Status: Need Info => Invalid
Open/Closed: Open => Closed

_______________________________________________________

Follow-up 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.

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.

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. :-)

_______________________________________________________

Reply to this item at:

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

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