[PATCH 3/3] maint: prohibit an operator at end of line

[Jim Meyering]

Here's a new syntax-check rule (patch 3/3).
1 and 2 fix violations so that the new check passes.

For now, this is only in coreutils (grep had only two violations),
but given the long list of violations in gnulib's lib/ directory,
it may not be generally appealing enough to be added to gnulib's maint.mk:

    $ git grep -lE '(. [-/+]|&&|\|\|)$' lib
    lib/acosl.c
    lib/c-strcasecmp.c
    lib/c-strncasecmp.c
    lib/file-has-acl.c
    lib/fnmatch_loop.c
    lib/fts.c
    lib/getaddrinfo.c
    lib/getcwd.c
    lib/gl_anyavltree_list2.h
    lib/gl_anyrbtree_list2.h
    lib/gl_avltree_oset.c
    lib/gl_rbtree_oset.c
    lib/idpriv.h
    lib/inet_ntop.c
    lib/inet_pton.c
    lib/mbscasecmp.c
    lib/mbsncasecmp.c
    lib/mktime.c
    lib/mountlist.c
    lib/poll.c
    lib/putenv.c
    lib/ref-add.sin
    lib/ref-del.sin
    lib/regcomp.c
    lib/regexec.c
    lib/select.c
    lib/sigpipe-die.c
    lib/sigpipe-die.h
    lib/sigprocmask.c
    lib/sincosl.c
    lib/strcasecmp.c
    lib/strncasecmp.c
    lib/strptime.c
    lib/strsignal.c
    lib/tanl.c


>From a08fcbb6a08b45cb33eeb42b41cb2c3d3818f756 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sun, 1 Jan 2012 01:46:34 +0100
Subject: [PATCH 1/3] maint: with split lines, don't leave an operator at end
 of line

Locate most offenders using this command:
  git grep -E '(. [-/+]|&&|\|\|)$' src
* src/copy.c (copy_reg): Split an expression before a binary operator,
not after it.
* src/cut.c (set_fields): Likewise.
* src/id.c (main): Likewise.
* src/install.c (setdefaultfilecon): Likewise.
* src/join.c (ignore_case): Likewise.
* src/pr.c (cols_ready_to_print, init_parameters, print_page): Likewise.
* src/stty.c (set_window_size): Likewise.
* src/wc.c (SUPPORT_OLD_MBRTOWC): Likewise.
* src/who.c (scan_entries): Likewise.
* src/test.c (binary_operator): Join a split line.
---
 src/copy.c    |    4 ++--
 src/cut.c     |    4 ++--
 src/id.c      |    4 ++--
 src/install.c |    4 ++--
 src/join.c    |    2 +-
 src/pr.c      |   22 +++++++++++-----------
 src/stty.c    |    4 ++--
 src/test.c    |    4 ++--
 src/wc.c      |    2 +-
 src/who.c     |    6 +++---
 10 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 414fbe0..26bbcf2 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1055,8 +1055,8 @@ copy_reg (char const *src_name, char const *dst_name,
                           make_holes, src_name, dst_name,
                           UINTMAX_MAX, &n_read,
                           &wrote_hole_at_eof)
-           || (wrote_hole_at_eof &&
-               ftruncate (dest_desc, n_read) < 0))
+           || (wrote_hole_at_eof
+               && ftruncate (dest_desc, n_read) < 0))
         {
           error (0, errno, _("failed to extend %s"), quote (dst_name));
           return_val = false;
diff --git a/src/cut.c b/src/cut.c
index fed9616..87380ac 100644
--- a/src/cut.c
+++ b/src/cut.c
@@ -372,8 +372,8 @@ set_fields (const char *fieldstr)
           initial = (lhs_specified ? value : 1);
           value = 0;
         }
-      else if (*fieldstr == ',' ||
-               isblank (to_uchar (*fieldstr)) || *fieldstr == '\0')
+      else if (*fieldstr == ','
+               || isblank (to_uchar (*fieldstr)) || *fieldstr == '\0')
         {
           in_digits = false;
           /* Ending the string, or this field/byte sublist. */
diff --git a/src/id.c b/src/id.c
index 158715e..41ae024 100644
--- a/src/id.c
+++ b/src/id.c
@@ -191,8 +191,8 @@ main (int argc, char **argv)
      invalid value that will be not displayed in print_full_info().  */
   if (selinux_enabled
       && n_ids == 0
-      && (just_context ||
-          (default_format && ! getenv ("POSIXLY_CORRECT"))))
+      && (just_context
+          || (default_format && ! getenv ("POSIXLY_CORRECT"))))
     {
       /* Report failure only if --context (-Z) was explicitly requested.  */
       if (getcon (&context) && just_context)
diff --git a/src/install.c b/src/install.c
index 5468d26..854436a 100644
--- a/src/install.c
+++ b/src/install.c
@@ -359,8 +359,8 @@ setdefaultfilecon (char const *file)

   /* If there's an error determining the context, or it has none,
      return to allow default context */
-  if ((matchpathcon (file, st.st_mode, &scontext) != 0) ||
-      STREQ (scontext, "<<none>>"))
+  if ((matchpathcon (file, st.st_mode, &scontext) != 0)
+      || STREQ (scontext, "<<none>>"))
     {
       if (scontext != NULL)
         freecon (scontext);
diff --git a/src/join.c b/src/join.c
index b92c1f8..e39ed87 100644
--- a/src/join.c
+++ b/src/join.c
@@ -173,7 +173,7 @@ static struct line uni_blank;
 /* If nonzero, ignore case when comparing join fields.  */
 static bool ignore_case;

-/* If nonzero, treat the first line of each file as column headers -
+/* If nonzero, treat the first line of each file as column headers --
    join them without checking for ordering */
 static bool join_header_lines;

diff --git a/src/pr.c b/src/pr.c
index f1e310f..2ea586d 100644
--- a/src/pr.c
+++ b/src/pr.c
@@ -781,9 +781,9 @@ cols_ready_to_print (void)

   n = 0;
   for (q = column_vector, i = 0; i < columns; ++q, ++i)
-    if (q->status == OPEN ||
-        q->status == FF_FOUND ||       /* With -b: To print a header only */
-        (storing_columns && q->lines_stored > 0 && q->lines_to_print > 0))
+    if (q->status == OPEN
+        || q->status == FF_FOUND       /* With -b: To print a header only */
+        || (storing_columns && q->lines_stored > 0 && q->lines_to_print > 0))
       ++n;
   return n;
 }
@@ -1275,13 +1275,13 @@ init_parameters (int number_of_files)

       /* To allow input tab-expansion (-e sensitive) use:
          if (number_separator == input_tab_char)
-           number_width = chars_per_number +
-             TAB_WIDTH (chars_per_input_tab, chars_per_number);   */
+           number_width = chars_per_number
+             + TAB_WIDTH (chars_per_input_tab, chars_per_number);   */

       /* Estimate chars_per_text without any margin and keep it constant. */
       if (number_separator == '\t')
-        number_width = chars_per_number +
-          TAB_WIDTH (chars_per_default_tab, chars_per_number);
+        number_width = (chars_per_number
+                        + TAB_WIDTH (chars_per_default_tab, chars_per_number));
       else
         number_width = chars_per_number + 1;

@@ -1297,8 +1297,8 @@ init_parameters (int number_of_files)
         power_10 = 10 * power_10;
     }

-  chars_per_column = (chars_per_line - chars_used_by_number -
-                     (columns - 1) * col_sep_length) / columns;
+  chars_per_column = (chars_per_line - chars_used_by_number
+                      - (columns - 1) * col_sep_length) / columns;

   if (chars_per_column < 1)
     error (EXIT_FAILURE, 0, _("page width too narrow"));
@@ -1836,8 +1836,8 @@ print_page (void)
                 {
                   if (empty_line)
                     align_empty_cols = true;
-                  else if (p->status == CLOSED ||
-                           (p->status == ON_HOLD && FF_only))
+                  else if (p->status == CLOSED
+                           || (p->status == ON_HOLD && FF_only))
                     align_column (p);
                 }
             }
diff --git a/src/stty.c b/src/stty.c
index 8e36593..eb07f85 100644
--- a/src/stty.c
+++ b/src/stty.c
@@ -1344,8 +1344,8 @@ set_window_size (int rows, int cols, char const 
*device_name)
      it's almost certainly a "struct winsize" instead.

      At any rate, the bug manifests itself when ws_row == 0; the symptom is
-     that ws_row is set to ws_col, and ws_col is set to (ws_xpixel<<16) +
-     ws_ypixel.  Since GNU stty sets rows and columns separately, this bug
+     that ws_row is set to ws_col, and ws_col is set to (ws_xpixel<<16)
+     + ws_ypixel.  Since GNU stty sets rows and columns separately, this bug
      caused "stty rows 0 cols 0" to set rows to cols and cols to 0, while
      "stty cols 0 rows 0" would do the right thing.  On a little-endian
      machine like the sun386i, the problem is the same, but for ws_col == 0.
diff --git a/src/test.c b/src/test.c
index 2d562d9..31a6420 100644
--- a/src/test.c
+++ b/src/test.c
@@ -367,8 +367,8 @@ binary_operator (bool l_is_l)
       test_syntax_error (_("unknown binary operator"), argv[op]);
     }

-  if (argv[op][0] == '=' && (!argv[op][1] ||
-       ((argv[op][1] == '=') && !argv[op][2])))
+  if (argv[op][0] == '='
+      && (!argv[op][1] || ((argv[op][1] == '=') && !argv[op][2])))
     {
       bool value = STREQ (argv[pos], argv[pos + 2]);
       pos += 3;
diff --git a/src/wc.c b/src/wc.c
index 1b8a7a4..5017377 100644
--- a/src/wc.c
+++ b/src/wc.c
@@ -292,7 +292,7 @@ wc (int fd, char const *file_x, struct fstatus *fstatus)
       /* Back-up the state before each multibyte character conversion and
          move the last incomplete character of the buffer to the front
          of the buffer.  This is needed because we don't know whether
-         the 'mbrtowc' function updates the state when it returns -2, -
+         the 'mbrtowc' function updates the state when it returns -2, --
          this is the ISO C 99 and glibc-2.2 behaviour - or not - amended
          ANSI C, glibc-2.1 and Solaris 5.7 behaviour.  We don't have an
          autoconf test for this, yet.  */
diff --git a/src/who.c b/src/who.c
index 84c68d7..c875b1d 100644
--- a/src/who.c
+++ b/src/who.c
@@ -590,9 +590,9 @@ scan_entries (size_t n, const STRUCT_UTMP *utmp_buf)

   while (n--)
     {
-      if (!my_line_only ||
-          strncmp (ttyname_b, utmp_buf->ut_line,
-                   sizeof (utmp_buf->ut_line)) == 0)
+      if (!my_line_only
+          || strncmp (ttyname_b, utmp_buf->ut_line,
+                      sizeof (utmp_buf->ut_line)) == 0)
         {
           if (need_users && IS_USER_PROCESS (utmp_buf))
             print_user (utmp_buf, boottime);
--
1.7.10.405.g10d433


>From 3ebe2d6866d3688627fce3edd72463073770940e Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 30 Apr 2012 10:55:18 +0200
Subject: [PATCH 2/3] maint: adjust wording in comments to avoid FP match on
 /\+$/

* src/ls.c: Also, "sortkey" is not a word: s/sortkey/sort key/.
---
 src/ls.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/ls.c b/src/ls.c
index 800f813..c54d80f 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -3443,13 +3443,13 @@ static int rev_xstrcoll_df_version (V a, V b)
 { DIRFIRST_CHECK (a, b); return cmp_version (b, a); }


-/* We have 2^3 different variants for each sortkey function
+/* We have 2^3 different variants for each sort-key function
    (for 3 independent sort modes).
    The function pointers stored in this array must be dereferenced as:

     sort_variants[sort_key][use_strcmp][reverse][dirs_first]

-   Note that the order in which sortkeys are listed in the function pointer
+   Note that the order in which sort keys are listed in the function pointer
    array below is defined by the order of the elements in the time_type and
    sort_type enums!  */

@@ -3493,14 +3493,14 @@ static qsortFunc const sort_functions[][2][2][2] =
     LIST_SORTFUNCTION_VARIANTS (atime)
   };

-/* The number of sortkeys is calculated as
-     the number of elements in the sort_type enum (i.e. sort_numtypes) +
+/* The number of sort keys is calculated as the sum of
+     the number of elements in the sort_type enum (i.e. sort_numtypes)
      the number of elements in the time_type enum (i.e. time_numtypes) - 1
    This is because when sort_type==sort_time, we have up to
-   time_numtypes possible sortkeys.
+   time_numtypes possible sort keys.

    This line verifies at compile-time that the array of sort functions has been
-   initialized for all possible sortkeys. */
+   initialized for all possible sort keys. */
 verify (ARRAY_CARDINALITY (sort_functions)
         == sort_numtypes + time_numtypes - 1 );

--
1.7.10.405.g10d433


>From ff5e5d0dd9b056b8920b62a96bb3eed1cec3bd50 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Mon, 30 Apr 2012 10:37:14 +0200
Subject: [PATCH 3/3] maint: prohibit an operator at end of line

Many coding standards, including GNU's, advocate that when
splitting a line near a binary operator, one should put the
operator at the beginning of the continued line, rather than
at the end of the preceding one.  This is for readability:
such operators are relatively important to readability, and
they are more apparent at the beginning of a line than
at the varying-column end of line,
* cfg.mk (sc_prohibit_operator_at_end_of_line): New rule.
Exempt test.c.
---
 cfg.mk |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/cfg.mk b/cfg.mk
index c3ddb42..1dfbb7d 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -200,6 +200,16 @@ sc_no_exec_perl_coreutils:
              exit 1; } || :;                                           \
        fi

+# With split lines, don't leave an operator at end of line.
+# Instead, put it on the following line, where it is more apparent.
+# Don't bother checking for "*" at end of line, since it provokes
+# far too many false positives, matching constructs like "TYPE *".
+sc_prohibit_operator_at_end_of_line:
+       @prohibit='. (&&|\|\||[-/+])$$'                                 \
+       in_vc_files='\.[chly]$$'                                        \
+       halt='found operator at end of line'                            \
+         $(_sc_search_regexp)
+
 # Don't use "readlink" or "readlinkat" directly
 sc_prohibit_readlink:
        @prohibit='\<readlink(at)? \('                                  \
@@ -456,3 +466,7 @@ 
exclude_file_name_regexp--sc_prohibit_continued_string_alpha_in_column_1 = \

 exclude_file_name_regexp--sc_prohibit_test_backticks = \
   ^tests/(init\.sh|check\.mk|misc/stdbuf)$$
+
+# Exempt test.c, since it's nominally shared, and relatively static.
+exclude_file_name_regexp--sc_prohibit_operator_at_end_of_line = \
+  ^src/test\.c$$
--
1.7.10.405.g10d433