This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[PATCH] add more style checks to check_GNU_style.sh
- From: Martin Sebor <msebor at gmail dot com>
- To: Gcc Patch List <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 25 Feb 2016 20:05:34 -0700
- Subject: [PATCH] add more style checks to check_GNU_style.sh
- Authentication-results: sourceware.org; auth=none
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_]\*)|([-%<=&|^?])|([^*]/)|([^:][+]))$'