On Fri, Sep 27, 2019 at 11:42:28AM +0100, Daniel P. Berrangé wrote:
On Fri, Sep 27, 2019 at 10:33:45AM +0100, Daniel P. Berrangé wrote:
On Thu, Sep 26, 2019 at 06:08:14PM +0200, Ján Tomko wrote:
On Tue, Sep 24, 2019 at 03:58:46PM +0100, Daniel P. Berrangé wrote:
As part of an goal to eliminate Perl from libvirt build tools, rewrite the check-spacing.pl tool in Python.
This was a straight conversion, manually going line-by-line to change the syntax from Perl to Python. Thus the overall structure of the file and approach is the same.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- Makefile.am | 2 +- build-aux/check-spacing.pl | 198 -------------------------------- build-aux/check-spacing.py | 229 +++++++++++++++++++++++++++++++++++++ cfg.mk | 4 +- 4 files changed, 232 insertions(+), 201 deletions(-) delete mode 100755 build-aux/check-spacing.pl create mode 100755 build-aux/check-spacing.py
diff --git a/build-aux/check-spacing.py b/build-aux/check-spacing.py new file mode 100755 index 0000000000..6b9f3ec1ba --- /dev/null +++ b/build-aux/check-spacing.py @@ -0,0 +1,229 @@ +#!/usr/bin/env python +# +# Copyright (C) 2012-2019 Red Hat, Inc. +# +# check-spacing.pl: Report any usage of 'function (..args..)' +# Also check for other syntax issues, such as correct use of ';' +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library. If not, see +# <http://www.gnu.org/licenses/>. + +from __future__ import print_function + +import re +import sys + + +def check_whitespace(filename): + errs = False + with open(filename, 'r') as fh: + quotedmetaprog = re.compile(r"""'[";,=]'""") + quotedstringprog = re.compile(r'''"(?:[^\\\"]|\\.)*"''') + commentstartprog = re.compile(r'''^(.*)/\*.*$''') + commentendprog = re.compile(r'''^.*\*/(.*)$''') + commentprog = re.compile(r'''^(.*)/\*.*\*/(.*)''') + funcprog = re.compile(r'''(\w+)\s\((?!\*)''') + keywordprog = re.compile( + r'''^.*\b(?:if|for|while|switch|return)\(.*$''') + functypedefprog = re.compile(r'''^.*\(\*\w+\)\s+\(.*$''') + whitespaceprog1 = re.compile(r'''^.*\s\).*$''') + whitespaceprog2 = re.compile(r'''^\s+\);?$''') + whitespaceprog3 = re.compile(r'''^.*\((?!$)\s.*''') + commasemiprog1 = re.compile(r'''.*\s[;,].*''') + commasemiprog2 = re.compile(r'''.*\S; ; .*''') + commasemiprog3 = re.compile(r'''^\s+;''') + semicolonprog = re.compile(r'''.*;[^ \\\n;)].*''') + commaprog = re.compile(r'''.*,[^ \\\n)}].*''') + assignprog1 = re.compile(r'''[^ ]\b[!<>&|\-+*\/%\^=]?=''') + assignprog2 = re.compile(r'''=[^= \\\n]''') + condstartprog = re.compile(r'''^\s*(if|while|for)\b.*\{$''') + statementprog = re.compile(r'''^[^;]*;[^;]*$''') + condendprog = re.compile(r'''^\s*}\s*$''') +
I think even with descriptive names for the regexes and the Python rewrite, this script is hard to read and comes too close to be a C parser.
Yeah, trying to parse C code using regular expressions was not our most sensible decision.
Also, the execution time goes from 1.5s to 6.5s, which is longer than all the other checks combined run on my 8-core machine.
Thanks for pointing that out. I'll see if there's any low hanging fruit to optimize but I'm doubtful.
I dont mind postponing this patch, as all patches in this series are independant of each other.
Can we get rid of it completely in favor of some proper formatting tool?
I think that would be a good idea because this script only handles a tiny subset of formatting rules we like.
I have played with clang-format to try to match our style, the main problems seem to be: * pre-processor directives are indented by the same offset as code (I see that cppi is specifically intended to add just one space)
That's an interesting approach. I wouldn't object to such indentation style myself.
* function calls sometimes leave an empty opening parenthesis
* always rewrapping function arguments might create unnecessary churn * parameters wrapping might not be smart enough, e.g. we like to do virReportError(VIR_ERR_CODE, "%s", _("string")); in a lot of places.
Yeah these last two points are the ones I struggled with too when I looked at clang-format 6 months back.
Another tool is "uncrustify" which seems to have even more configurability than clang-format does. On my SSD it takes 30 seconds to run over all the .c file.
I support both of the tools, as we can remove our own syntax-check to some extent and some remaining parts can be rewritten into python. This would help a lot with the Meson rewrite that I'm working on now. I also played with "uncrustify" and it looks reasonable and with some tweaking we could get close to our current coding style only with some tiny changes but clang-format works for me as well. Pavel