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@xxxxxxxxxx> > > > > --- > > > > 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
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
