This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] add more style checks to check_GNU_style.sh


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  <msebor@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.pop@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_]\*)|([-%<=&|^?])|([^*]/)|([^:][+]))$'

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]