[Grep-devel] avoiding new warnings

[Jim Meyering]

Using latest gnulib (manywarnings update) with gcc7 and configuring with
--enable-gcc-warnings exposed some build failures:

  - /* fallthrough */ comments are inadequate, so use FALLTHROUGH macro
  - -Wduplicated-branches complained about three false positives,
      so I've wrapped each in a new warning-ignoring macro.

I've pushed the following three patches so that grep continues to
build that way with the latest gnulib.

>From 9e2c89397916d9587f1945528eb5d5651c0b4edb Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 20 May 2017 17:45:36 -0700
Subject: [PATCH 1/3] maint: update to work with GCC7's
 -Werror=implicit-fallthrough=

* src/system.h (FALLTHROUGH): Define.
* src/grep.c (context_length_arg): Use new FALLTHROUGH macro in place
of comments
(fgrep_to_grep_pattern, try_fgrep_pattern, main): Likewise.
---
 src/grep.c   | 10 +++++-----
 src/system.h |  8 ++++++++
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/src/grep.c b/src/grep.c
index 775f227..1958e26 100644
--- a/src/grep.c
+++ b/src/grep.c
@@ -759,7 +759,7 @@ context_length_arg (char const *str, intmax_t *out)
     case LONGINT_OVERFLOW:
       if (0 <= *out)
         break;
-      /* Fall through.  */
+      FALLTHROUGH;
     default:
       die (EXIT_TROUBLE, 0, "%s: %s", str,
            _("invalid context length argument"));
@@ -2308,7 +2308,7 @@ fgrep_to_grep_pattern (char **keys_p, size_t *len_p)
         {
         case (size_t) -2:
           n = len;
-          /* Fall through.  */
+          FALLTHROUGH;
         default:
           p = mempcpy (p, keys, n);
           break;
@@ -2316,7 +2316,7 @@ fgrep_to_grep_pattern (char **keys_p, size_t *len_p)
         case (size_t) -1:
           memset (&mb_state, 0, sizeof mb_state);
           n = 1;
-          /* Fall through.  */
+          FALLTHROUGH;
         case 1:
           switch (*keys)
             {
@@ -2376,7 +2376,7 @@ try_fgrep_pattern (int matcher, char *keys, size_t *len_p)
               case '(': case '+': case '?': case '{': case '|':
                 if (matcher == G_MATCHER_INDEX)
                   goto fail;
-                /* Fall through.  */
+                FALLTHROUGH;
               default:
                 q++, len--;
                 break;
@@ -2653,7 +2653,7 @@ main (int argc, char **argv)

       case 'R':
         fts_options = basic_fts_options | FTS_LOGICAL;
-        /* Fall through.  */
+        FALLTHROUGH;
       case 'r':
         directories = RECURSE_DIRECTORIES;
         last_recursive = prev_optind;
diff --git a/src/system.h b/src/system.h
index 6292a28..f974793 100644
--- a/src/system.h
+++ b/src/system.h
@@ -107,4 +107,12 @@ static _GL_UNUSED void
 __asan_unpoison_memory_region (void const volatile *addr, size_t size) { }
 #endif

+#ifndef FALLTHROUGH
+# if __GNUC__ < 7
+#  define FALLTHROUGH ((void) 0)
+# else
+#  define FALLTHROUGH __attribute__ ((__fallthrough__))
+# endif
+#endif
+
 #endif
-- 
2.9.4


>From 1e1e23358050f201f5630a16609c441b9e7955a0 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 21 May 2017 09:23:18 -0700
Subject: [PATCH 2/3] maint: accommodate GCC7's -Werror=duplicated-branches

* src/system.h (IGNORE_DUPLICATE_BRANCH_WARNING): Define.
* src/grep.c (grepfile): Use it.
* src/kwset.c (bmexec, acexec): Use it.
---
 src/grep.c   |  3 ++-
 src/kwset.c  | 15 ++++++++-------
 src/system.h | 14 ++++++++++++++
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/src/grep.c b/src/grep.c
index 1958e26..bc2599f 100644
--- a/src/grep.c
+++ b/src/grep.c
@@ -1700,7 +1700,8 @@ static bool
 grepfile (int dirdesc, char const *name, bool follow, bool command_line)
 {
   int oflag = (O_RDONLY | O_NOCTTY
-               | (binary ? O_BINARY : 0)
+               | (IGNORE_DUPLICATE_BRANCH_WARNING
+                  (binary ? O_BINARY : 0))
                | (follow ? 0 : O_NOFOLLOW)
                | (skip_devices (command_line) ? O_NONBLOCK : 0));
   int desc = openat_safer (dirdesc, name, oflag);
diff --git a/src/kwset.c b/src/kwset.c
index 2599af5..a2ce6b6 100644
--- a/src/kwset.c
+++ b/src/kwset.c
@@ -756,10 +756,10 @@ bmexec (kwset_t kwset, char const *text, ptrdiff_t size,
 {
   /* Help the compiler inline in two ways, depending on whether
      kwset->trans is null.  */
-  ptrdiff_t ret = (kwset->trans
-                   ? bmexec_trans (kwset, text, size)
-                   : bmexec_trans (kwset, text, size));
-
+  ptrdiff_t ret = (IGNORE_DUPLICATE_BRANCH_WARNING
+                   (kwset->trans
+                    ? bmexec_trans (kwset, text, size)
+                    : bmexec_trans (kwset, text, size)));
   if (0 <= ret)
     {
        kwsmatch->index = 0;
@@ -905,9 +905,10 @@ acexec (kwset_t kwset, char const *text, ptrdiff_t size,
   assume (0 <= size);
   /* Help the compiler inline in two ways, depending on whether
      kwset->trans is null.  */
-  return (kwset->trans
-          ? acexec_trans (kwset, text, size, kwsmatch, longest)
-          : acexec_trans (kwset, text, size, kwsmatch, longest));
+  return (IGNORE_DUPLICATE_BRANCH_WARNING
+          (kwset->trans
+           ? acexec_trans (kwset, text, size, kwsmatch, longest)
+           : acexec_trans (kwset, text, size, kwsmatch, longest)));
 }

 /* Find the first instance of a KWSET member in TEXT, which has SIZE bytes.
diff --git a/src/system.h b/src/system.h
index f974793..bac7823 100644
--- a/src/system.h
+++ b/src/system.h
@@ -115,4 +115,18 @@ __asan_unpoison_memory_region (void const volatile *addr, 
size_t size) { }
 # endif
 #endif

+/* When we deliberately use duplicate branches, use this macro to
+   disable gcc's -Wduplicated-branches in the containing expression.  */
+#if 7 <= __GNUC__
+# define IGNORE_DUPLICATE_BRANCH_WARNING(exp)                          \
+  ({                                                                   \
+    _Pragma ("GCC diagnostic push")                                    \
+    _Pragma ("GCC diagnostic ignored \"-Wduplicated-branches\"")       \
+    exp;                                                               \
+    _Pragma ("GCC diagnostic pop")                                     \
+  })
+#else
+# define IGNORE_DUPLICATE_BRANCH_WARNING(exp) exp
+#endif
+
 #endif
-- 
2.9.4


>From 1ad2bc71689c49e42e38f75c9ab9e53796a95977 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 20 May 2017 17:31:32 -0700
Subject: [PATCH 3/3] gnulib: update to latest; and update tests/init.sh

---
 gnulib        |  2 +-
 tests/init.sh | 92 +++++++++++++++++++++++++++--------------------------------
 2 files changed, 43 insertions(+), 51 deletions(-)

diff --git a/gnulib b/gnulib
index 8e2bc0b..88033d3 160000
--- a/gnulib
+++ b/gnulib
@@ -1 +1 @@
-Subproject commit 8e2bc0b51c8e58e4c20a99ea21c84bcd6ad9d495
+Subproject commit 88033d3779362aad8fd11345636d9578f94c14d7
diff --git a/tests/init.sh b/tests/init.sh
index 6fc4a4d..584194f 100644
--- a/tests/init.sh
+++ b/tests/init.sh
@@ -45,6 +45,9 @@
 # Running a single test, with verbose output:
 #   $ make check TESTS=test-foo.sh VERBOSE=yes
 #
+# Running a single test, keeping the temporary directory:
+#   $ make check TESTS=test-foo.sh KEEP=yes
+#
 # Running a single test, with single-stepping:
 #   1. Go into a sub-shell:
 #   $ bash
@@ -128,6 +131,13 @@ else
 fi

 # We require $(...) support unconditionally.
+# We require non-surprising "local" semantics (this eliminates dash).
+# This takes the admittedly draconian step of eliminating dash, because the
+# assignment tab=$(printf '\t') works fine, yet preceding it with "local "
+# transforms it into an assignment that sets the variable to the empty string.
+# That is too counter-intuitive, and can lead to subtle run-time malfunction.
+# The example below is less subtle in that with dash, it evokes the run-time
+# exception "dash: 1: local: 1: bad variable name".
 # We require a few additional shell features only when $EXEEXT is nonempty,
 # in order to support automatic $EXEEXT emulation:
 # - hyphen-containing alias names
@@ -151,6 +161,7 @@ fi
 gl_shell_test_script_='
 test $(echo y) = y || exit 1
 f_local_() { local v=1; }; f_local_ || exit 1
+f_dash_local_fail_() { local t=$(printf " 1"); }; f_dash_local_fail_
 score_=10
 if test "$VERBOSE" = yes; then
   test -n "$( (exec 3>&1; set -x; P=1 true 2>&3) 2> /dev/null)" && score_=9
@@ -287,44 +298,24 @@ compare_dev_null_ ()
   return 2
 }

-if diff_out_=`exec 2>/dev/null; diff -u "$0" "$0" < /dev/null` \
-   && diff -u Makefile "$0" 2>/dev/null | grep '^[+]#!' >/dev/null; then
-  # diff accepts the -u option and does not (like AIX 7 'diff') produce an
-  # extra space on column 1 of every content line.
-  if test -z "$diff_out_"; then
-    compare_ () { diff -u "$@"; }
-  else
-    compare_ ()
-    {
-      if diff -u "$@" > diff.out; then
-        # No differences were found, but Solaris 'diff' produces output
-        # "No differences encountered". Hide this output.
-        rm -f diff.out
-        true
-      else
-        cat diff.out
-        rm -f diff.out
-        false
-      fi
-    }
-  fi
-elif diff_out_=`exec 2>/dev/null; diff -c "$0" "$0" < /dev/null`; then
+for diff_opt_ in -u -U3 -c '' no; do
+  test "$diff_opt_" != no &&
+    diff_out_=`exec 2>/dev/null; diff $diff_opt_ "$0" "$0" < /dev/null` &&
+    break
+done
+if test "$diff_opt_" != no; then
   if test -z "$diff_out_"; then
-    compare_ () { diff -c "$@"; }
+    compare_ () { diff $diff_opt_ "$@"; }
   else
     compare_ ()
     {
-      if diff -c "$@" > diff.out; then
-        # No differences were found, but AIX and HP-UX 'diff' produce output
-        # "No differences encountered" or "There are no differences between the
-        # files.". Hide this output.
-        rm -f diff.out
-        true
-      else
-        cat diff.out
-        rm -f diff.out
-        false
-      fi
+      # If no differences were found, AIX and HP-UX 'diff' produce output
+      # like "No differences encountered".  Hide this output.
+      diff $diff_opt_ "$@" > diff.out
+      diff_status_=$?
+      test $diff_status_ -eq 0 || cat diff.out || diff_status_=2
+      rm -f diff.out || diff_status_=2
+      return $diff_status_
     }
   fi
 elif cmp -s /dev/null /dev/null 2>/dev/null; then
@@ -361,11 +352,15 @@ remove_tmp_ ()
 {
   __st=$?
   cleanup_
-  # cd out of the directory we're about to remove
-  cd "$initial_cwd_" || cd / || cd /tmp
-  chmod -R u+rwx "$test_dir_"
-  # If removal fails and exit status was to be 0, then change it to 1.
-  rm -rf "$test_dir_" || { test $__st = 0 && __st=1; }
+  if test "$KEEP" = yes; then
+    echo "Not removing temporary directory $test_dir_"
+  else
+    # cd out of the directory we're about to remove
+    cd "$initial_cwd_" || cd / || cd /tmp
+    chmod -R u+rwx "$test_dir_"
+    # If removal fails and exit status was to be 0, then change it to 1.
+    rm -rf "$test_dir_" || { test $__st = 0 && __st=1; }
+  fi
   exit $__st
 }

@@ -466,7 +461,6 @@ setup_ ()
   fi

   initial_cwd_=$PWD
-  fail=0

   pfx_=`testdir_prefix_`
   test_dir_=`mktempd_ "$initial_cwd_" "$pfx_-$ME_.XXXX"` \
@@ -550,8 +544,9 @@ mktempd_ ()
   # Disallow any trailing slash on specified destdir:
   # it would subvert the post-mktemp "case"-based destdir test.
   case $destdir_ in
-  /) ;;
+  / | //) destdir_slash_=$destdir;;
   */) fail_ "invalid destination dir: remove trailing slash(es)";;
+  *) destdir_slash_=$destdir_/;;
   esac

   case $template_ in
@@ -561,20 +556,17 @@ mktempd_ ()
   esac

   # First, try to use mktemp.
-  d=`unset TMPDIR; { mktemp -d -t -p "$destdir_" "$template_"; } 2>/dev/null` \
-    || fail=1
+  d=`unset TMPDIR; { mktemp -d -t -p "$destdir_" "$template_"; } 2>/dev/null` 
&&

   # The resulting name must be in the specified directory.
-  case $d in "$destdir_"*);; *) fail=1;; esac
+  case $d in "$destdir_slash_"*) :;; *) false;; esac &&

   # It must have created the directory.
-  test -d "$d" || fail=1
+  test -d "$d" &&

   # It must have 0700 permissions.  Handle sticky "S" bits.
-  perms=`ls -dgo "$d" 2>/dev/null|tr S -` || fail=1
-  case $perms in drwx------*) ;; *) fail=1;; esac
-
-  test $fail = 0 && {
+  perms=`ls -dgo "$d" 2>/dev/null` &&
+  case $perms in drwx--[-S]---*) :;; *) false;; esac && {
     echo "$d"
     return
   }
@@ -593,7 +585,7 @@ mktempd_ ()
   i_=1
   while :; do
     X_=`rand_bytes_ $nx_`
-    candidate_dir_="$destdir_/$base_template_$X_"
+    candidate_dir_="$destdir_slash_$base_template_$X_"
     err_=`mkdir -m 0700 "$candidate_dir_" 2>&1` \
       && { echo "$candidate_dir_"; return; }
     test $MAX_TRIES_ -le $i_ && break;
-- 
2.9.4