Recently I had the opportunity to learn about a number of rather subtle style conventions sometimes enforced during code reviews (though not inconsistently followed in GCC code). To help find these kinds of problems before a patch is submitted and cut down on the subsequent back-and-forth, I've added checks for some of these conventions to the check_GNU_style.sh script. In addition, since the script tends to produce lots of noise for things that can't be fixed in tests like lines in excess of 80 characters caused by dg-error directives, I tweaked it to suppress these.
To verify that the new checkers don't add too much noise I ran the patched script on the last 1000 commits, both with and without testsuite changes (all without libstdc++). The results are below. It's telling that over a quarter of all commits violate even the limited subset of the GNU coding guidelines the script checks for. script w/o tests w/tests baseline 258 434 patched 282 466 FWIW, to make the script more usable (e.g., to check the rules appropriate for each file type, including documentation), it will need to be considerably enhanced. I think most of it can be done in AWK (already used by the script), and I might try to tackle that at some point in the future. Martin
contrib/ChangeLog: 2016-02-25 Martin Sebor <mse...@redhat.com> * check_GNU_style.sh: Add a new global variable. Add checks for trailing operators and spaces before left brackets. Tightened up a check for a trailing left curly brace. (g, ag, vg): Use color. (col): Don't complain about excessively long lines with DejaGnu directives. Index: contrib/check_GNU_style.sh =================================================================== --- contrib/check_GNU_style.sh (revision 233722) +++ contrib/check_GNU_style.sh (working copy) @@ -1,7 +1,7 @@ #!/bin/sh # Checks some of the GNU style formatting rules in a set of patches. -# Copyright (C) 2010, 2012 Free Software Foundation, Inc. +# Copyright (C) 2010, 2012, 2016 Free Software Foundation, Inc. # Contributed by Sebastian Pop <sebastian....@amd.com> # This program is free software; you can redistribute it and/or modify @@ -15,8 +15,11 @@ # GNU General Public License for more details. # You should have received a copy of the GNU General Public License -# along with this program; if not, write to the Free Software -# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +# along with this program; if not, see the file COPYING3. If not, +# see <http://www.gnu.org/licenses/>. + +# Set to empty in the environment to override. +: ${color:---color=always} usage() { cat <<EOF @@ -100,7 +103,7 @@ g (){ local found=false cat $inp \ - | egrep --color=always -- "$arg" \ + | egrep $color -- "$arg" \ > "$tmp" && found=true if $found; then @@ -117,8 +120,8 @@ ag (){ local found=false cat $inp \ - | egrep --color=always -- "$arg1" \ - | egrep --color=always -- "$arg2" \ + | egrep $color -- "$arg1" \ + | egrep $color -- "$arg2" \ > "$tmp" && found=true if $found; then @@ -136,7 +139,7 @@ vg (){ local found=false cat $inp \ | egrep -v -- "$varg" \ - | egrep --color=always -- "$arg" \ + | egrep $color -- "$arg" \ > "$tmp" && found=true if $found; then @@ -171,10 +174,11 @@ col (){ # Expand tabs to spaces according to tab positions. # Keep long lines, make short lines empty. Print the part past 80 chars # in red. + # Don't complain about dg-xxx directives in tests. cat "$tmp" \ | sed 's/^[0-9]*:+//' \ | expand \ - | awk '{ \ + | awk '$0 !~ /{[[:space:]]*dg-(error|warning|message)[[:space:]]/ { \ if (length($0) > 80) \ printf "%s\033[1;31m%s\033[0m\n", \ substr($0,1,80), \ @@ -201,6 +205,7 @@ col (){ done } + col 'Lines should not exceed 80 characters.' g 'Blocks of 8 spaces should be replaced with tabs.' \ @@ -221,13 +226,20 @@ g 'Dot, space, space, end of comment.' \ g 'Sentences should end with a dot. Dot, space, space, end of the comment.' \ '[[:alnum:]][[:blank:]]*\*/' -vg 'There should be exactly one space between function name and parentheses.' \ +vg 'There should be exactly one space between function name and parenthesis.' \ '\#define' \ '[[:alnum:]]([[:blank:]]{2,})?\(' -g 'There should be no space before closing parentheses.' \ +g 'There should be no space before a left square bracket.' \ + '[[:alnum:]][[:blank:]]+\[' + +g 'There should be no space before closing parenthesis.' \ '[[:graph:]][[:blank:]]+\)' -ag 'Braces should be on a separate line.' \ - '\{' \ - 'if[[:blank:]]\(|while[[:blank:]]\(|switch[[:blank:]]\(' +# This will give false positives for C99 compound literals. +g 'Braces should be on a separate line.' \ + '(\)|else)[[:blank:]]*{' + +# Does this apply to definition of aggregate objects? +g 'Trailing operator.' \ + '(([^a-zA-Z_]\*)|([-%<=&|^?])|([^*]/)|([^:][+]))$'
