Fix -Wempty-body warnings in glibc

Message ID alpine.DEB.2.21.1902072306340.10857@digraph.polyomino.org.uk
State Committed
Headers

Commit Message

Joseph Myers Feb. 7, 2019, 11:07 p.m. UTC
  One group of warnings seen building glibc with -Wextra is -Wempty-body
warnings about an 'if' body (or in one case an 'else' body) that is
just a semicolon, "warning: suggest braces around empty body in an
'if' statement [-Wempty-body]" - I think the point of the warning
being to make it more visible whether an 'if' body is actually present
or not.

This patch fixes such warnings in glibc.  There's one place, with a
semicolon at the end of a comment, where this is clearly making the
presence of an 'else' body more visible.  The other cases involve
macro definitions expanding to nothing.  While there's no issue there
with visibility at the call sites, I think it's still cleaner to have
a macro that expands to something nonempty appropriate for the context
- so do {} while (0) if it's only intended to be usable as a
statement, or ((void) 0) where the macro definition is an alternative
to a call to a function returning void, so this patch makes those
changes.

Tested for x86_64.

2019-02-07  Joseph Myers  <joseph@codesourcery.com>

	* catgets/gencat.c (normalize_line): Use braces around empty
	'else' body.
	* include/stap-probe.h [!USE_STAP_PROBE && !__ASSEMBLER__]
	(STAP_PROBE0): Use do {} while (0) for do-nothing definition.
	[!USE_STAP_PROBE && !__ASSEMBLER__] (STAP_PROBE1): Likewise.
	[!USE_STAP_PROBE && !__ASSEMBLER__] (STAP_PROBE2): Likewise.
	[!USE_STAP_PROBE && !__ASSEMBLER__] (STAP_PROBE3): Likewise.
	[!USE_STAP_PROBE && !__ASSEMBLER__] (STAP_PROBE4): Likewise.
	* libio/libio.h (_IO_funlockfile): Use ((void) 0) for do-nothing
	definition.
  

Comments

Joseph Myers Feb. 11, 2019, 6:23 p.m. UTC | #1
Ping.  This patch 
<https://sourceware.org/ml/libc-alpha/2019-02/msg00200.html> is pending 
review.
  
Gabriel F. T. Gomes Feb. 13, 2019, 1:43 p.m. UTC | #2
On Thu, Feb 07 2019, Joseph Myers wrote:
> 
> This patch fixes such warnings in glibc.  There's one place, with a
> semicolon at the end of a comment, where this is clearly making the
> presence of an 'else' body more visible.  The other cases involve
> macro definitions expanding to nothing.  While there's no issue there
> with visibility at the call sites, I think it's still cleaner to have
> a macro that expands to something nonempty appropriate for the context
> - so do {} while (0) if it's only intended to be usable as a
> statement, or ((void) 0) where the macro definition is an alternative
> to a call to a function returning void, so this patch makes those
> changes.

I agree that the readability of the code is already good enough at the
call sites, and that adding braces to them wouldn't make it better.

Your patch looks good to me.

> -		/* Simply ignore the backslash character.  */;
> +		{
> +		  /* Simply ignore the backslash character.  */
> +		}

OK.  We don't just remove the else block entirely because we want the
comment.

> -#  define STAP_PROBE0()
> -#  define STAP_PROBE1(a1)
> -#  define STAP_PROBE2(a1, a2)
> -#  define STAP_PROBE3(a1, a2, a3)
> -#  define STAP_PROBE4(a1, a2, a3, a4)
> +#  define STAP_PROBE0()				do {} while (0)
> +#  define STAP_PROBE1(a1)			do {} while (0)
> +#  define STAP_PROBE2(a1, a2)			do {} while (0)
> +#  define STAP_PROBE3(a1, a2, a3)		do {} while (0)
> +#  define STAP_PROBE4(a1, a2, a3, a4)		do {} while (0)

OK.

> -#define _IO_funlockfile(_fp) /**/
> +#define _IO_funlockfile(_fp) ((void) 0)

In the call sites, such as in _IO_acquire_lock_fct (libio/libioP.h),
'_IO_funlockfile (foo);' was getting expanded to '/**/;', thus causing
the warning.

I think your change maintains the nice readability of the code at the
call sites, so it looks good to me.
  

Patch

diff --git a/catgets/gencat.c b/catgets/gencat.c
index 9d6db7017e..51fd2a724e 100644
--- a/catgets/gencat.c
+++ b/catgets/gencat.c
@@ -1196,7 +1196,9 @@  normalize_line (const char *fname, size_t line, iconv_t cd, wchar_t *string,
 		  ++rp;
 		}
 	      else
-		/* Simply ignore the backslash character.  */;
+		{
+		  /* Simply ignore the backslash character.  */
+		}
 	      break;
 	    }
       }
diff --git a/include/stap-probe.h b/include/stap-probe.h
index 8c26292edd..85f41c9162 100644
--- a/include/stap-probe.h
+++ b/include/stap-probe.h
@@ -60,11 +60,11 @@ 
 /* Evaluate all the arguments and verify that N matches their number.  */
 #  define LIBC_PROBE(name, n, ...) STAP_PROBE##n (__VA_ARGS__)
 
-#  define STAP_PROBE0()
-#  define STAP_PROBE1(a1)
-#  define STAP_PROBE2(a1, a2)
-#  define STAP_PROBE3(a1, a2, a3)
-#  define STAP_PROBE4(a1, a2, a3, a4)
+#  define STAP_PROBE0()				do {} while (0)
+#  define STAP_PROBE1(a1)			do {} while (0)
+#  define STAP_PROBE2(a1, a2)			do {} while (0)
+#  define STAP_PROBE3(a1, a2, a3)		do {} while (0)
+#  define STAP_PROBE4(a1, a2, a3, a4)		do {} while (0)
 
 # else
 #  define LIBC_PROBE(name, n, ...)		/* Nothing.  */
diff --git a/libio/libio.h b/libio/libio.h
index d21162aab0..9c08c7eba1 100644
--- a/libio/libio.h
+++ b/libio/libio.h
@@ -240,7 +240,7 @@  extern int _IO_ftrylockfile (FILE *) __THROW;
 
 #define _IO_peekc(_fp) _IO_peekc_unlocked (_fp)
 #define _IO_flockfile(_fp) /**/
-#define _IO_funlockfile(_fp) /**/
+#define _IO_funlockfile(_fp) ((void) 0)
 #define _IO_ftrylockfile(_fp) /**/
 #ifndef _IO_cleanup_region_start
 #define _IO_cleanup_region_start(_fct, _fp) /**/