5:51 a.m.
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(a)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