Discussion:
[PATCH] fallthrough: update for GCC 7/8
(too old to reply)
Jim Meyering
2017-05-20 00:12:42 UTC
Permalink
Raw Message
When building diffutils with latest gnulib and gcc.git, the
fallthrough-like comments did not suppress warnings, so I've
converted most such instances to use the new attribute,
via the FALLTHROUGH macro.

This has meant adding four more definitions of the FALLTHROUGH macro.
Eventually, we should consider putting it somewhere common and perhaps
renaming it to have the GL_ prefix.

I have not yet converted the two remaining uses,

lib/regcomp.c:2324: /* FALLTHROUGH */
lib/regexec.c:4081: /* FALLTHROUGH */

because they're in shared code and not causing trouble for diffutils.
However, this will come up soon with sed, grep and coreutils.

Subject: [PATCH] fallthrough: update for GCC 7/8

* lib/quotearg.c (FALLTHROUGH): New macro.
Use it whenever one switch case falls through into the next,
replacing "/* Fall through */" comments. This exposed one
instance of an unwarranted "fall through" comment: unwarranted
because it preceded a "goto" label not a case statement.
* lib/freopen-safer.c (freopen_safer): Likewise.
* lib/fts.c (leaf_optimization_applies): Likewise.
* lib/unistr/u8-uctomb-aux.c (u8_uctomb_aux): Likewise.
* tests/test-getopt_long.h (getopt_long_loop): Likewise.
* tests/test-tsearch.c (mangle_tree): Likewise. Also include
tests/macros.h for the definition.
* tests/test-argp.c (group1_parser): Likewise.
* tests/test-getopt.h (getopt_loop): Likewise.
---
ChangeLog | 17 +++++++++++++++++
lib/freopen-safer.c | 14 +++++++++++---
lib/fts.c | 12 ++++++++++--
lib/quotearg.c | 23 +++++++++++++++--------
lib/unistr/u8-uctomb-aux.c | 12 ++++++++++--
tests/test-argp.c | 4 +++-
tests/test-getopt.h | 3 ++-
tests/test-getopt_long.h | 2 +-
tests/test-tsearch.c | 4 +++-
9 files changed, 72 insertions(+), 19 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 46714cd..807c8ed 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,20 @@
+2017-05-18 Jim Meyering <***@fb.com>
+
+ fallthrough: update for GCC 7/8
+ * lib/quotearg.c (FALLTHROUGH): New macro.
+ Use it whenever one switch case falls through into the next,
+ replacing "/* Fall through */" comments. This exposed one
+ instance of an unwarranted "fall through" comment: unwarranted
+ because it preceded a "goto" label not a case statement.
+ * lib/freopen-safer.c (freopen_safer): Likewise.
+ * lib/fts.c (leaf_optimization_applies): Likewise.
+ * lib/unistr/u8-uctomb-aux.c (u8_uctomb_aux): Likewise.
+ * tests/test-getopt_long.h (getopt_long_loop): Likewise.
+ * tests/test-tsearch.c (mangle_tree): Likewise. Also include
+ tests/macros.h for the definition.
+ * tests/test-argp.c (group1_parser): Likewise.
+ * tests/test-getopt.h (getopt_loop): Likewise.
+
2017-05-19 Paul Eggert <***@cs.ucla.edu>

argp: fix shift bug
diff --git a/lib/freopen-safer.c b/lib/freopen-safer.c
index 42dc39c..be84ae6 100644
--- a/lib/freopen-safer.c
+++ b/lib/freopen-safer.c
@@ -26,6 +26,14 @@
#include <stdbool.h>
#include <unistd.h>

+#ifndef FALLTHROUGH
+# if __GNUC__ < 7
+# define FALLTHROUGH ((void) 0)
+# else
+# define FALLTHROUGH __attribute__ ((__fallthrough__))
+# endif
+#endif
+
/* Guarantee that FD is open; all smaller FDs must already be open.
Return true if successful. */
static bool
@@ -69,15 +77,15 @@ freopen_safer (char const *name, char const *mode, FILE *f)
default: /* -1 or not a standard stream. */
if (dup2 (STDERR_FILENO, STDERR_FILENO) != STDERR_FILENO)
protect_err = true;
- /* fall through */
+ FALLTHROUGH;
case STDERR_FILENO:
if (dup2 (STDOUT_FILENO, STDOUT_FILENO) != STDOUT_FILENO)
protect_out = true;
- /* fall through */
+ FALLTHROUGH;
case STDOUT_FILENO:
if (dup2 (STDIN_FILENO, STDIN_FILENO) != STDIN_FILENO)
protect_in = true;
- /* fall through */
+ FALLTHROUGH;
case STDIN_FILENO:
/* Nothing left to protect. */
break;
diff --git a/lib/fts.c b/lib/fts.c
index fabaa97..3bac324 100644
--- a/lib/fts.c
+++ b/lib/fts.c
@@ -203,6 +203,14 @@ enum Fts_stat
while (false)
#endif

+#ifndef FALLTHROUGH
+# if __GNUC__ < 7
+# define FALLTHROUGH ((void) 0)
+# else
+# define FALLTHROUGH __attribute__ ((__fallthrough__))
+# endif
+#endif
+
static FTSENT *fts_alloc (FTS *, const char *, size_t) internal_function;
static FTSENT *fts_build (FTS *, int) internal_function;
static void fts_lfree (FTSENT *) internal_function;
@@ -732,11 +740,11 @@ leaf_optimization_applies (int dir_fd)
of large directories, so as per <https://bugzilla.redhat.com/1252549>
NFS should return true. However st_nlink values are not accurate on
all implementations as per <https://bugzilla.redhat.com/1299169>. */
- /* fall through */
+ FALLTHROUGH;
case S_MAGIC_PROC:
/* Per <http://bugs.debian.org/143111> /proc may have
bogus stat.st_nlink values. */
- /* fall through */
+ FALLTHROUGH;
default:
return false;
}
diff --git a/lib/quotearg.c b/lib/quotearg.c
index 26903c7..f657033 100644
--- a/lib/quotearg.c
+++ b/lib/quotearg.c
@@ -54,6 +54,14 @@

#define INT_BITS (sizeof (int) * CHAR_BIT)

+#ifndef FALLTHROUGH
+# if __GNUC__ < 7
+# define FALLTHROUGH ((void) 0)
+# else
+# define FALLTHROUGH __attribute__ ((__fallthrough__))
+# endif
+#endif
+
struct quoting_options
{
/* Basic quoting style. */
@@ -310,7 +318,7 @@ quotearg_buffer_restyled (char *buffer, size_t buffersize,
case c_maybe_quoting_style:
quoting_style = c_quoting_style;
elide_outer_quotes = true;
- /* Fall through. */
+ FALLTHROUGH;
case c_quoting_style:
if (!elide_outer_quotes)
STORE ('"');
@@ -365,14 +373,14 @@ quotearg_buffer_restyled (char *buffer, size_t buffersize,

case shell_escape_quoting_style:
backslash_escapes = true;
- /* Fall through. */
+ FALLTHROUGH;
case shell_quoting_style:
elide_outer_quotes = true;
- /* Fall through. */
+ FALLTHROUGH;
case shell_escape_always_quoting_style:
if (!elide_outer_quotes)
backslash_escapes = true;
- /* Fall through. */
+ FALLTHROUGH;
case shell_always_quoting_style:
quoting_style = shell_always_quoting_style;
if (!elide_outer_quotes)
@@ -505,7 +513,6 @@ quotearg_buffer_restyled (char *buffer, size_t buffersize,
if (quoting_style == shell_always_quoting_style
&& elide_outer_quotes)
goto force_outer_quoting_style;
- /* Fall through. */
c_escape:
if (backslash_escapes)
{
@@ -517,14 +524,14 @@ quotearg_buffer_restyled (char *buffer, size_t buffersize,
case '{': case '}': /* sometimes special if isolated */
if (! (argsize == SIZE_MAX ? arg[1] == '\0' : argsize == 1))
break;
- /* Fall through. */
+ FALLTHROUGH;
case '#': case '~':
if (i != 0)
break;
- /* Fall through. */
+ FALLTHROUGH;
case ' ':
c_and_shell_quote_compat = true;
- /* Fall through. */
+ FALLTHROUGH;
case '!': /* special in bash */
case '"': case '$': case '&':
case '(': case ')': case '*': case ';':
diff --git a/lib/unistr/u8-uctomb-aux.c b/lib/unistr/u8-uctomb-aux.c
index b6adbe0..72d35d8 100644
--- a/lib/unistr/u8-uctomb-aux.c
+++ b/lib/unistr/u8-uctomb-aux.c
@@ -20,6 +20,14 @@
/* Specification. */
#include "unistr.h"

+#ifndef FALLTHROUGH
+# if __GNUC__ < 7
+# define FALLTHROUGH ((void) 0)
+# else
+# define FALLTHROUGH __attribute__ ((__fallthrough__))
+# endif
+#endif
+
int
u8_uctomb_aux (uint8_t *s, ucs4_t uc, int n)
{
@@ -48,9 +56,9 @@ u8_uctomb_aux (uint8_t *s, ucs4_t uc, int n)
switch (count) /* note: code falls through cases! */
{
case 4: s[3] = 0x80 | (uc & 0x3f); uc = uc >> 6; uc |= 0x10000;
- /* fallthrough */
+ FALLTHROUGH;
case 3: s[2] = 0x80 | (uc & 0x3f); uc = uc >> 6; uc |= 0x800;
- /* fallthrough */
+ FALLTHROUGH;
case 2: s[1] = 0x80 | (uc & 0x3f); uc = uc >> 6; uc |= 0xc0;
/*case 1:*/ s[0] = uc;
}
diff --git a/tests/test-argp.c b/tests/test-argp.c
index fe9c415..04a0a90 100644
--- a/tests/test-argp.c
+++ b/tests/test-argp.c
@@ -26,6 +26,8 @@
# include <strings.h>
#endif

+#include "macros.h"
+
struct test_args
{
int test;
@@ -63,7 +65,7 @@ group1_parser (int key, char *arg, struct argp_state *state)

case 'r':
args->read = 1;
- /* fall through */
+ FALLTHROUGH;
case 'f':
args->file = arg;
break;
diff --git a/tests/test-getopt.h b/tests/test-getopt.h
index ce470c8..5c0e5b5 100644
--- a/tests/test-getopt.h
+++ b/tests/test-getopt.h
@@ -17,6 +17,7 @@
/* Written by Bruno Haible <***@clisp.org>, 2009. */

#include <stdbool.h>
+#include "macros.h"

/* The glibc/gnulib implementation of getopt supports setting optind =
0, but not all other implementations do. This matters for getopt.
@@ -66,7 +67,7 @@ getopt_loop (int argc, const char **argv,
ASSERT (options[0] == ':'
|| ((options[0] == '-' || options[0] == '+')
&& options[1] == ':'));
- /* fall through */
+ FALLTHROUGH;
case '?':
*unrecognized = optopt;
break;
diff --git a/tests/test-getopt_long.h b/tests/test-getopt_long.h
index 670ef9c..e90ebed 100644
--- a/tests/test-getopt_long.h
+++ b/tests/test-getopt_long.h
@@ -87,7 +87,7 @@ getopt_long_loop (int argc, const char **argv,
ASSERT (options[0] == ':'
|| ((options[0] == '-' || options[0] == '+')
&& options[1] == ':'));
- /* fall through */
+ FALLTHROUGH;
case '?':
*unrecognized = optopt;
break;
diff --git a/tests/test-tsearch.c b/tests/test-tsearch.c
index 5cbf38d..9e77524 100644
--- a/tests/test-tsearch.c
+++ b/tests/test-tsearch.c
@@ -34,6 +34,8 @@ SIGNATURE_CHECK (twalk, void, (void const *,
#include <stdlib.h>
#include <string.h>

+#include "macros.h"
+
#define SEED 0
#if HAVE_TSEARCH
/* The system's tsearch() is not expected to keep the tree balanced. */
@@ -234,8 +236,7 @@ mangle_tree (enum order how, enum action what, void **root, int lag)
break;

j = k;
- /* fall through */
+ FALLTHROUGH;

case delete:
elem = tfind (x + j, (void *const *) root, cmp_fn);
--
2.9.4
Bruno Haible
2017-05-20 10:12:47 UTC
Permalink
Raw Message
Post by Jim Meyering
diff --git a/tests/test-getopt.h b/tests/test-getopt.h
index ce470c8..5c0e5b5 100644
--- a/tests/test-getopt.h
+++ b/tests/test-getopt.h
@@ -17,6 +17,7 @@
#include <stdbool.h>
+#include "macros.h"
/* The glibc/gnulib implementation of getopt supports setting optind =
0, but not all other implementations do. This matters for getopt.
This is not necessary, as it's already included in tests/test-getopt-main.h,
before test-getopt.h gets included.


2017-05-20 Bruno Haible <***@clisp.org>

getopt-posix tests: Remove redundant include.
* tests/test-getopt.h: Don't include "macros.h". It's already included
by tests/test-getopt-main.h.

diff --git a/tests/test-getopt.h b/tests/test-getopt.h
index 5c0e5b5..a41fdc1 100644
--- a/tests/test-getopt.h
+++ b/tests/test-getopt.h
@@ -17,7 +17,6 @@
/* Written by Bruno Haible <***@clisp.org>, 2009. */

#include <stdbool.h>
-#include "macros.h"

/* The glibc/gnulib implementation of getopt supports setting optind =
0, but not all other implementations do. This matters for getopt.
Bruno Haible
2017-05-20 10:15:00 UTC
Permalink
Raw Message
Post by Jim Meyering
* tests/test-tsearch.c (mangle_tree): Likewise. Also include
tests/macros.h for the definition.
* tests/test-argp.c (group1_parser): Likewise.
The module descriptions need to be adjusted accordingly:


2017-05-20 Bruno Haible <***@clisp.org>

argp, tsearch tests: Fix file list.
* modules/argp-tests (Files): Add tests/macros.h.
* modules/tsearch-tests (Files): Likewise.

diff --git a/modules/argp-tests b/modules/argp-tests
index ce621d9..fada682 100644
--- a/modules/argp-tests
+++ b/modules/argp-tests
@@ -1,6 +1,7 @@
Files:
tests/test-argp.c
tests/test-argp-2.sh
+tests/macros.h

Depends-on:

diff --git a/modules/tsearch-tests b/modules/tsearch-tests
index ca32649..2612b72 100644
--- a/modules/tsearch-tests
+++ b/modules/tsearch-tests
@@ -2,6 +2,7 @@ Files:
tests/test-tsearch.sh
tests/test-tsearch.c
tests/signature.h
+tests/macros.h

Depends-on:
stdint
Jim Meyering
2017-05-20 15:12:33 UTC
Permalink
Raw Message
Post by Bruno Haible
Post by Jim Meyering
* tests/test-tsearch.c (mangle_tree): Likewise. Also include
tests/macros.h for the definition.
* tests/test-argp.c (group1_parser): Likewise.
argp, tsearch tests: Fix file list.
* modules/argp-tests (Files): Add tests/macros.h.
* modules/tsearch-tests (Files): Likewise.
Thank you for the quick fixes.
Paul Eggert
2017-05-20 20:15:29 UTC
Permalink
Raw Message
Eventually, we should consider putting it [FALLTHROUGH] somewhere common and perhaps
renaming it to have the GL_ prefix.
The former sounds good. The latter, I'm more dubious about, as the macro is
called FALLTHROUGH in bleeding-edge Emacs source code (admittedly thanks to
yours truly). I expect that in practice it would not make much sense nowadays to
define the identifier FALLTHROUGH to anything else, given that C++17 has
standardized the [[fallthrough]] attribute.
Bernhard Voelker
2017-05-22 06:07:14 UTC
Permalink
Raw Message
Post by Jim Meyering
* lib/quotearg.c (FALLTHROUGH): New macro.
Use it whenever one switch case falls through into the next,
replacing "/* Fall through */" comments. This exposed one
instance of an unwarranted "fall through" comment: unwarranted
because it preceded a "goto" label not a case statement.
Here's a diff output with one more line of context:

@@ -504,9 +512,8 @@ quotearg_buffer_restyled (char *buffer, size_t buffersize,
c_and_shell_escape:
if (quoting_style == shell_always_quoting_style
&& elide_outer_quotes)
goto force_outer_quoting_style;
- /* Fall through. */
c_escape:
if (backslash_escapes)
{
c = esc;

IMO the "fall through" comment was warranted ... in the else case.

Have a nice day,
Berny
Pádraig Brady
2017-05-22 11:50:08 UTC
Permalink
Raw Message
Post by Bernhard Voelker
Post by Jim Meyering
* lib/quotearg.c (FALLTHROUGH): New macro.
Use it whenever one switch case falls through into the next,
replacing "/* Fall through */" comments. This exposed one
instance of an unwarranted "fall through" comment: unwarranted
because it preceded a "goto" label not a case statement.
@@ -504,9 +512,8 @@ quotearg_buffer_restyled (char *buffer, size_t buffersize,
if (quoting_style == shell_always_quoting_style
&& elide_outer_quotes)
goto force_outer_quoting_style;
- /* Fall through. */
if (backslash_escapes)
{
c = esc;
IMO the "fall through" comment was warranted ... in the else case.
Agreed. Pushed that change in your name.
Thanks to both of you for the cleanups.
Jim Meyering
2017-05-22 15:06:40 UTC
Permalink
Raw Message
Post by Pádraig Brady
Post by Bernhard Voelker
Post by Jim Meyering
* lib/quotearg.c (FALLTHROUGH): New macro.
Use it whenever one switch case falls through into the next,
replacing "/* Fall through */" comments. This exposed one
instance of an unwarranted "fall through" comment: unwarranted
because it preceded a "goto" label not a case statement.
@@ -504,9 +512,8 @@ quotearg_buffer_restyled (char *buffer, size_t buffersize,
if (quoting_style == shell_always_quoting_style
&& elide_outer_quotes)
goto force_outer_quoting_style;
- /* Fall through. */
if (backslash_escapes)
{
c = esc;
IMO the "fall through" comment was warranted ... in the else case.
Agreed. Pushed that change in your name.
Thanks to both of you for the cleanups.
Indeed. Thanks.

Loading...