Eric Blake wrote: > On 04/30/2012 08:25 AM, Eric Blake wrote: >> On 04/30/2012 07:08 AM, Jim Meyering wrote: >>> 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 >> >> Is it worth adding some more operators, such as *, ^, &, and |? >> >> (. [-/+*^]|&&?|\|\|?)$ > > Also <<, >>, ==, !=, and for that matter, all of the op= operators: > > (. [-/+*^=!<>]=?|&&?|\|\|?|<<=?|>>=?)$
Good point. Here's a more complete patch that also makes the regular expression overridable. I'm still omitting "*" and "=" from the default. >From b8a6996e258a2c30de40fb20cab0d17a38c3eff2 Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> 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 * 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/extent-scan.c (extent_scan_read): Move an ">" from end of line to beginning of the following. Likewise for two other expressions. --- src/copy.c | 4 ++-- src/cut.c | 4 ++-- src/extent-scan.c | 12 ++++++------ 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 +++--- 11 files changed, 34 insertions(+), 34 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/extent-scan.c b/src/extent-scan.c index e269f54..0c25c57 100644 --- a/src/extent-scan.c +++ b/src/extent-scan.c @@ -133,11 +133,11 @@ extent_scan_read (struct extent_scan *scan) unsigned int i = 0; for (i = 0; i < fiemap->fm_mapped_extents; i++) { - assert (fm_extents[i].fe_logical <= - OFF_T_MAX - fm_extents[i].fe_length); + assert (fm_extents[i].fe_logical + <= OFF_T_MAX - fm_extents[i].fe_length); - if (si && last_ei->ext_flags == - (fm_extents[i].fe_flags & ~FIEMAP_EXTENT_LAST) + if (si && last_ei->ext_flags + == (fm_extents[i].fe_flags & ~FIEMAP_EXTENT_LAST) && (last_ei->ext_logical + last_ei->ext_length == fm_extents[i].fe_logical)) { @@ -147,8 +147,8 @@ extent_scan_read (struct extent_scan *scan) last_ei->ext_flags = fm_extents[i].fe_flags; } else if ((si == 0 && scan->scan_start > fm_extents[i].fe_logical) - || (si && last_ei->ext_logical + last_ei->ext_length > - fm_extents[i].fe_logical)) + || (si && (last_ei->ext_logical + last_ei->ext_length + > fm_extents[i].fe_logical))) { /* BTRFS before 2.6.38 could return overlapping extents for sparse files. We adjust the returned extents 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.420.g9768c >From 57e0a882ef70712f79e7255aa5c1f8ba27b4c33e Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> Date: Mon, 30 Apr 2012 10:55:18 +0200 Subject: [PATCH 2/3] maint: adjust comments to avoid FP match on binary-operator-at-EOL * src/ls.c (print_long_format): Reformat comment to avoid "==" at end of line. Also, "sortkey" is not a word: s/sortkey/sort key/. * src/ioblksize.h: Likewise, for "|" from a shell snippet. * src/runcon.c: Likewise, for "|" in grammar-like usage. --- src/ioblksize.h | 4 ++-- src/ls.c | 20 ++++++++++---------- src/runcon.c | 6 +++--- 3 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/ioblksize.h b/src/ioblksize.h index ffd6ff3..aaea9ff 100644 --- a/src/ioblksize.h +++ b/src/ioblksize.h @@ -28,8 +28,8 @@ bs=$((1024*2**$i)) printf "%7s=" $bs timeout --foreground -sINT 2 \ - dd bs=$bs if=/dev/zero of=/dev/null 2>&1 | - sed -n 's/.* \([0-9.]* [GM]B\/s\)/\1/p' + dd bs=$bs if=/dev/zero of=/dev/null 2>&1 \ + | sed -n 's/.* \([0-9.]* [GM]B\/s\)/\1/p' done With the results shown for these systems: diff --git a/src/ls.c b/src/ls.c index 800f813..397e4ea 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 ); @@ -3918,10 +3918,10 @@ print_long_format (const struct fileinfo *f) gettime (¤t_time); } - /* Consider a time to be recent if it is within the past six - months. A Gregorian year has 365.2425 * 24 * 60 * 60 == - 31556952 seconds on the average. Write this value as an - integer constant to avoid floating point hassles. */ + /* Consider a time to be recent if it is within the past six months. + A Gregorian year has 365.2425 * 24 * 60 * 60 == 31556952 seconds + on the average. Write this value as an integer constant to + avoid floating point hassles. */ six_months_ago.tv_sec = current_time.tv_sec - 31556952 / 2; six_months_ago.tv_nsec = current_time.tv_nsec; diff --git a/src/runcon.c b/src/runcon.c index 29bf0e5..875441f 100644 --- a/src/runcon.c +++ b/src/runcon.c @@ -15,9 +15,9 @@ along with this program. If not, see <http://www.gnu.org/licenses/>. */ /* - * runcon [ context | - * ( [ -c ] [ -r role ] [-t type] [ -u user ] [ -l levelrange ] ) - * command [arg1 [arg2 ...] ] + * runcon [ context + * | ( [ -c ] [ -r role ] [-t type] [ -u user ] [ -l levelrange ] ) + * command [arg1 [arg2 ...] ] * * attempt to run the specified command with the specified context. * -- 1.7.10.420.g9768c >From e744f4b7b7de9b76fdd314aa406e9fb150b9168d Mon Sep 17 00:00:00 2001 From: Jim Meyering <[email protected]> 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 and head.c. --- cfg.mk | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/cfg.mk b/cfg.mk index c3ddb42..923785e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -200,6 +200,18 @@ 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 *". +# Similarly, omit "=" (initializers). +binop_re_ ?= [-/+^!<>]|[-/+*^!<>=]=|&&?|\|\|?|<<=?|>>=? +sc_prohibit_operator_at_end_of_line: + @prohibit='. ($(binop_re_))$$' \ + 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 +468,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/(ptx|test|head)\.c$$ -- 1.7.10.420.g9768c
