[libvirt] [PATCH 0/9] syntax-check: make spacing check faster

Simplify some regexes to increase coverage and speed up the execution. Reduces (git ls-files "*.c" | xargs build-aux/bracket-spacing.pl) by half a second (from 1.9s to 1.33s), the effect on complete syntax-check is only ~150 ms. Ján Tomko (9): vbox: reformat multi-line error reports maint: remove whitespace from closing parentheses Rename virAssertCmpInt to testAssertEq Rename bracket-spacing.pl to check-spacing.pl check-spacing: rewrite whitespace check before (semi)colon check-spacing: rewrite regex for checking the closing parenthesis check-spacing: simplify keyword spacing check check-spacing: remove virAssertCmpInt exception check-spacing: use non-capturing groups build-aux/bracket-spacing.pl | 204 ------------------------------------------ build-aux/check-spacing.pl | 204 ++++++++++++++++++++++++++++++++++++++++++ cfg.mk | 6 +- src/qemu/qemu_command.c | 3 +- src/vbox/vbox_common.c | 8 +- src/vbox/vbox_snapshot_conf.c | 68 +++++++------- src/vz/vz_driver.c | 3 +- src/vz/vz_sdk.c | 3 +- tests/viratomictest.c | 52 +++++------ 9 files changed, 274 insertions(+), 277 deletions(-) delete mode 100755 build-aux/bracket-spacing.pl create mode 100755 build-aux/check-spacing.pl -- 2.7.3

Put the comma on the first line. --- src/vbox/vbox_common.c | 8 ++--- src/vbox/vbox_snapshot_conf.c | 68 +++++++++++++++++++++---------------------- 2 files changed, 38 insertions(+), 38 deletions(-) diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c index 8f79f0a..6dd5b9c 100644 --- a/src/vbox/vbox_common.c +++ b/src/vbox/vbox_common.c @@ -4367,8 +4367,8 @@ static int vboxCloseDisksRecursively(virDomainPtr dom, char *location) } rc = gVBoxAPI.UIMedium.GetChildren(medium, &childrenSize, &children); if (NS_FAILED(rc)) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s" - , _("Unable to get disk children")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to get disk children")); goto cleanup; } for (i = 0; i < childrenSize; i++) { @@ -4385,8 +4385,8 @@ static int vboxCloseDisksRecursively(virDomainPtr dom, char *location) VBOX_UTF16_TO_UTF8(childLocationUtf, &childLocation); VBOX_UTF16_FREE(childLocationUtf); if (vboxCloseDisksRecursively(dom, childLocation) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s" - , _("Unable to close disk children")); + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to close disk children")); goto cleanup; } VIR_FREE(childLocation); diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c index 2520a02..e1307e1 100644 --- a/src/vbox/vbox_snapshot_conf.c +++ b/src/vbox/vbox_snapshot_conf.c @@ -70,8 +70,8 @@ virVBoxSnapshotConfCreateVBoxSnapshotConfHardDiskPtr(xmlNodePtr diskNode, 1, &searchTabResult); if (resultSize != 1) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <HardDisk> 'uuid' attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <HardDisk> 'uuid' attribute")); goto cleanup; } if (VIR_STRDUP(hardDisk->uuid, searchTabResult[0]) < 0) @@ -79,8 +79,8 @@ virVBoxSnapshotConfCreateVBoxSnapshotConfHardDiskPtr(xmlNodePtr diskNode, location = virXMLPropString(diskNode, "location"); if (location == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <HardDisk> 'location' attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <HardDisk> 'location' attribute")); goto cleanup; } if (location[0] != '/') { @@ -95,8 +95,8 @@ virVBoxSnapshotConfCreateVBoxSnapshotConfHardDiskPtr(xmlNodePtr diskNode, } hardDisk->format = virXMLPropString(diskNode, "format"); if (hardDisk->format == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <HardDisk> 'format' attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <HardDisk> 'format' attribute")); goto cleanup; } hardDisk->type = virXMLPropString(diskNode, "type"); @@ -204,8 +204,8 @@ virVBoxSnapshotConfRetrieveSnapshot(xmlNodePtr snapshotNode, 1, &searchTabResult); if (resultSize != 1) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Snapshot> 'uuid' attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Snapshot> 'uuid' attribute")); goto cleanup; } if (VIR_STRDUP(snapshot->uuid, searchTabResult[0]) < 0) @@ -213,14 +213,14 @@ virVBoxSnapshotConfRetrieveSnapshot(xmlNodePtr snapshotNode, snapshot->name = virXMLPropString(snapshotNode, "name"); if (snapshot->name == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Snapshot> 'name' attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Snapshot> 'name' attribute")); goto cleanup; } snapshot->timeStamp = virXMLPropString(snapshotNode, "timeStamp"); if (snapshot->timeStamp == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Snapshot> 'timeStamp' attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Snapshot> 'timeStamp' attribute")); goto cleanup; } @@ -231,16 +231,16 @@ virVBoxSnapshotConfRetrieveSnapshot(xmlNodePtr snapshotNode, hardwareNode = virXPathNode("./vbox:Hardware", xPathContext); if (hardwareNode == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Snapshot> <Hardware> node")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Snapshot> <Hardware> node")); goto cleanup; } snapshot->hardware = virXMLNodeToString(snapshotNode->doc, hardwareNode); storageControllerNode = virXPathNode("./vbox:StorageControllers", xPathContext); if (storageControllerNode == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Snapshot> <StorageControllers> node")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Snapshot> <StorageControllers> node")); goto cleanup; } snapshot->storageController = virXMLNodeToString(snapshotNode->doc, @@ -633,21 +633,21 @@ virVBoxSnapshotConfLoadVboxFile(const char *filePath, xPathContext->node = cur; machineNode = virXPathNode("./vbox:Machine", xPathContext); if (machineNode == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <VirtualBox> <Machine> node")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <VirtualBox> <Machine> node")); goto cleanup; } machineDescription->uuid = virXMLPropString(machineNode, "uuid"); if (machineDescription->uuid == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Machine> 'uuid' attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Machine> 'uuid' attribute")); goto cleanup; } machineDescription->name = virXMLPropString(machineNode, "name"); if (machineDescription->name == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Machine> 'name' attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Machine> 'name' attribute")); goto cleanup; } @@ -659,8 +659,8 @@ virVBoxSnapshotConfLoadVboxFile(const char *filePath, 1, &searchResultTab); if (searchResultSize != 1) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Machine> 'currentSnapshot' attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Machine> 'currentSnapshot' attribute")); goto cleanup; } if (VIR_STRDUP(machineDescription->currentSnapshot, searchResultTab[0]) < 0) @@ -669,8 +669,8 @@ virVBoxSnapshotConfLoadVboxFile(const char *filePath, machineDescription->snapshotFolder = virXMLPropString(machineNode, "snapshotFolder"); if (machineDescription->snapshotFolder == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Machine> 'snapshotFolder' attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Machine> 'snapshotFolder' attribute")); goto cleanup; } @@ -682,16 +682,16 @@ virVBoxSnapshotConfLoadVboxFile(const char *filePath, } machineDescription->lastStateChange = virXMLPropString(machineNode, "lastStateChange"); if (machineDescription->lastStateChange == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Machine> 'lastStateChange' attribute")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Machine> 'lastStateChange' attribute")); goto cleanup; } xPathContext->node = machineNode; cur = virXPathNode("./vbox:Hardware", xPathContext); if (cur == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Machine> <Hardware> node")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Machine> <Hardware> node")); goto cleanup; } machineDescription->hardware = virXMLNodeToString(xml, cur); @@ -702,8 +702,8 @@ virVBoxSnapshotConfLoadVboxFile(const char *filePath, cur = virXPathNode("./vbox:StorageControllers", xPathContext); if (cur == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Machine> <StorageControllers> node")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Machine> <StorageControllers> node")); goto cleanup; } machineDescription->storageController = virXMLNodeToString(xml, cur); @@ -711,8 +711,8 @@ virVBoxSnapshotConfLoadVboxFile(const char *filePath, /*retrieve mediaRegistry*/ cur = virXPathNode("./vbox:MediaRegistry", xPathContext); if (cur == NULL) { - virReportError(VIR_ERR_XML_ERROR, "%s" - , _("Cannot parse <Machine> <MediaRegistry> node")); + virReportError(VIR_ERR_XML_ERROR, "%s", + _("Cannot parse <Machine> <MediaRegistry> node")); goto cleanup; } machineDescription->mediaRegistry = virVBoxSnapshotConfRetrieveMediaRegistry(cur, xPathContext, machineLocation); -- 2.7.3

On Wed, Jun 15, 2016 at 12:06:50 +0200, Ján Tomko wrote:
Put the comma on the first line. --- src/vbox/vbox_common.c | 8 ++--- src/vbox/vbox_snapshot_conf.c | 68 +++++++++++++++++++++---------------------- 2 files changed, 38 insertions(+), 38 deletions(-)
Yuck!!! ACK

To allow tightening syntax check. --- src/qemu/qemu_command.c | 3 +-- src/vz/vz_driver.c | 3 +-- src/vz/vz_sdk.c | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 48be399..cbd40ea 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4178,8 +4178,7 @@ qemuBuildVideoCommandLine(virCommandPtr cmd, (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_QXL && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_QXL_VGA)) || (primaryVideoType == VIR_DOMAIN_VIDEO_TYPE_VIRTIO && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU))) - ) { + virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_VIRTIO_GPU)))) { for (i = 0; i < def->nvideos; i++) { char *str; virCommandAddArg(cmd, "-device"); diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index bb96fa0..67017c2 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -321,8 +321,7 @@ vzDriverObjNew(void) !(driver->domainEventState = virObjectEventStateNew()) || (vzInitVersion(driver) < 0) || (prlsdkConnect(driver) < 0) || - (prlsdkSubscribeToPCSEvents(driver) < 0) - ) { + (prlsdkSubscribeToPCSEvents(driver) < 0)) { virObjectUnref(driver); return NULL; } diff --git a/src/vz/vz_sdk.c b/src/vz/vz_sdk.c index 99c5d4a..586c656 100644 --- a/src/vz/vz_sdk.c +++ b/src/vz/vz_sdk.c @@ -2266,8 +2266,7 @@ prlsdkCheckUnsupportedParams(PRL_HANDLE sdkdom, virDomainDefPtr def) !((def->inputs[0]->type == VIR_DOMAIN_INPUT_TYPE_MOUSE && def->inputs[1]->type == VIR_DOMAIN_INPUT_TYPE_KBD) || (def->inputs[0]->type == VIR_DOMAIN_INPUT_TYPE_KBD && - def->inputs[1]->type == VIR_DOMAIN_INPUT_TYPE_MOUSE)) - ) { + def->inputs[1]->type == VIR_DOMAIN_INPUT_TYPE_MOUSE))) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("unsupported input device configuration")); -- 2.7.3

On Wed, Jun 15, 2016 at 12:06:51 +0200, Ján Tomko wrote:
To allow tightening syntax check. --- src/qemu/qemu_command.c | 3 +-- src/vz/vz_driver.c | 3 +-- src/vz/vz_sdk.c | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-)
ACK

Drop the op parameter, we only use equality. Drop the vir prefix since it's only used in the tests. --- tests/viratomictest.c | 52 +++++++++++++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/tests/viratomictest.c b/tests/viratomictest.c index 634f5ab..5185045 100644 --- a/tests/viratomictest.c +++ b/tests/viratomictest.c @@ -35,72 +35,72 @@ testTypes(const void *data ATTRIBUTE_UNUSED) int s, s2; bool res; -#define virAssertCmpInt(a, op, b) \ - if (!(a op b)) \ +#define testAssertEq(a, b) \ + if (!(a == b)) \ return -1; virAtomicIntSet(&u, 5); u2 = virAtomicIntGet(&u); - virAssertCmpInt(u2, ==, 5); + testAssertEq(u2, 5); res = virAtomicIntCompareExchange(&u, 6, 7); if (res) return -1; - virAssertCmpInt(u, ==, 5); + testAssertEq(u, 5); - virAssertCmpInt(virAtomicIntAdd(&u, 1), ==, 5); - virAssertCmpInt(u, ==, 6); + testAssertEq(virAtomicIntAdd(&u, 1), 5); + testAssertEq(u, 6); - virAssertCmpInt(virAtomicIntInc(&u), ==, 7); - virAssertCmpInt(u, ==, 7); + testAssertEq(virAtomicIntInc(&u), 7); + testAssertEq(u, 7); res = virAtomicIntDecAndTest(&u); if (res) return -1; - virAssertCmpInt(u, ==, 6); + testAssertEq(u, 6); u2 = virAtomicIntAnd(&u, 5); - virAssertCmpInt(u2, ==, 6); - virAssertCmpInt(u, ==, 4); + testAssertEq(u2, 6); + testAssertEq(u, 4); u2 = virAtomicIntOr(&u, 8); - virAssertCmpInt(u2, ==, 4); - virAssertCmpInt(u, ==, 12); + testAssertEq(u2, 4); + testAssertEq(u, 12); u2 = virAtomicIntXor(&u, 4); - virAssertCmpInt(u2, ==, 12); - virAssertCmpInt(u, ==, 8); + testAssertEq(u2, 12); + testAssertEq(u, 8); virAtomicIntSet(&s, 5); s2 = virAtomicIntGet(&s); - virAssertCmpInt(s2, ==, 5); + testAssertEq(s2, 5); res = virAtomicIntCompareExchange(&s, 6, 7); if (res) return -1; - virAssertCmpInt(s, ==, 5); + testAssertEq(s, 5); virAtomicIntAdd(&s, 1); - virAssertCmpInt(s, ==, 6); + testAssertEq(s, 6); virAtomicIntInc(&s); - virAssertCmpInt(s, ==, 7); + testAssertEq(s, 7); res = virAtomicIntDecAndTest(&s); if (res) return -1; - virAssertCmpInt(s, ==, 6); + testAssertEq(s, 6); s2 = virAtomicIntAnd(&s, 5); - virAssertCmpInt(s2, ==, 6); - virAssertCmpInt(s, ==, 4); + testAssertEq(s2, 6); + testAssertEq(s, 4); s2 = virAtomicIntOr(&s, 8); - virAssertCmpInt(s2, ==, 4); - virAssertCmpInt(s, ==, 12); + testAssertEq(s2, 4); + testAssertEq(s, 12); s2 = virAtomicIntXor(&s, 4); - virAssertCmpInt(s2, ==, 12); - virAssertCmpInt(s, ==, 8); + testAssertEq(s2, 12); + testAssertEq(s, 8); return 0; } -- 2.7.3

On Wed, Jun 15, 2016 at 12:06:52 +0200, Ján Tomko wrote:
Drop the op parameter, we only use equality. Drop the vir prefix since it's only used in the tests. --- tests/viratomictest.c | 52 +++++++++++++++++++++++++-------------------------- 1 file changed, 26 insertions(+), 26 deletions(-)
ACK

We test whitespace with lots of other characters now. --- build-aux/bracket-spacing.pl | 204 ------------------------------------------- build-aux/check-spacing.pl | 204 +++++++++++++++++++++++++++++++++++++++++++ cfg.mk | 6 +- 3 files changed, 207 insertions(+), 207 deletions(-) delete mode 100755 build-aux/bracket-spacing.pl create mode 100755 build-aux/check-spacing.pl diff --git a/build-aux/bracket-spacing.pl b/build-aux/bracket-spacing.pl deleted file mode 100755 index 5bc96d2..0000000 --- a/build-aux/bracket-spacing.pl +++ /dev/null @@ -1,204 +0,0 @@ -#!/usr/bin/perl -# -# bracket-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/>. -# -# Authors: -# Daniel P. Berrange <berrange@redhat.com> - -use strict; -use warnings; - -my $ret = 0; -my $incomment = 0; - -foreach my $file (@ARGV) { - # Per-file variables for multiline Curly Bracket (cb_) check - my $cb_linenum = 0; - my $cb_code = ""; - my $cb_scolon = 0; - - open FILE, $file; - - while (defined (my $line = <FILE>)) { - my $data = $line; - # For temporary modifications - my $tmpdata; - - # Kill any quoted , ; = or " - $data =~ s/'[";,=]'/'X'/g; - - # Kill any quoted strings - $data =~ s,"([^\\\"]|\\.)*","XXX",g; - - # Kill any C++ style comments - $data =~ s,//.*$,//,; - - next if $data =~ /^#/; - - # Kill contents of multi-line comments - # and detect end of multi-line comments - if ($incomment) { - if ($data =~ m,\*/,) { - $incomment = 0; - $data =~ s,^.*\*/,*/,; - } else { - $data = ""; - } - } - - # Kill single line comments, and detect - # start of multi-line comments - if ($data =~ m,/\*.*\*/,) { - $data =~ s,/\*.*\*/,/* */,; - } elsif ($data =~ m,/\*,) { - $incomment = 1; - $data =~ s,/\*.*,/*,; - } - - # We need to match things like - # - # int foo (int bar, bool wizz); - # foo (bar, wizz); - # - # but not match things like: - # - # typedef int (*foo)(bar wizz) - # - # we can't do this (efficiently) without - # missing things like - # - # foo (*bar, wizz); - # - # We also don't want to spoil the $data so it can be used - # later on. - $tmpdata = $data; - while ($tmpdata =~ /(\w+)\s\((?!\*)/) { - my $kw = $1; - - # Allow space after keywords only - if ($kw =~ /^(if|for|while|switch|return)$/) { - $tmpdata =~ s/($kw\s\()/XXX(/; - } else { - print "Whitespace after non-keyword:\n"; - print "$file:$.: $line"; - $ret = 1; - last; - } - } - - # Require whitespace immediately after keywords, - # but none after the opening bracket - if ($data =~ /\b(if|for|while|switch|return)\(/ || - $data =~ /\b(if|for|while|switch|return)\s+\(\s/) { - print "No whitespace after keyword:\n"; - print "$file:$.: $line"; - $ret = 1; - } - - # Forbid whitespace between )( of a function typedef - if ($data =~ /\(\*\w+\)\s+\(/) { - print "Whitespace between ')' and '(':\n"; - print "$file:$.: $line"; - $ret = 1; - } - - # Forbid whitespace following ( or prior to ) - if ($data =~ /\S\s+\)/ || - $data =~ /\(\s+\S/) { - print "Whitespace after '(' or before ')':\n"; - print "$file:$.: $line"; - $ret = 1; - } - - # Forbid whitespace before ";" or ",". Things like below are allowed: - # - # 1) The expression is empty for "for" loop. E.g. - # for (i = 0; ; i++) - # - # 2) An empty statement. E.g. - # while (write(statuswrite, &status, 1) == -1 && - # errno == EINTR) - # ; - # - if ($data =~ /[^;\s]\s+[;,]/) { - print "Whitespace before (semi)colon:\n"; - print "$file:$.: $line"; - $ret = 1; - } - - # Require EOL, macro line continuation, or whitespace after ";". - # Allow "for (;;)" as an exception. - if ($data =~ /;[^ \\\n;)]/) { - print "Invalid character after semicolon:\n"; - print "$file:$.: $line"; - $ret = 1; - } - - # Require EOL, space, or enum/struct end after comma. - if ($data =~ /,[^ \\\n)}]/) { - print "Invalid character after comma:\n"; - print "$file:$.: $line"; - $ret = 1; - } - - # Require spaces around assignment '=', compounds and '==' - # with the exception of virAssertCmpInt() - $tmpdata = $data; - $tmpdata =~ s/(virAssertCmpInt\(.* ).?=,/$1op,/; - if ($tmpdata =~ /[^ ]\b[!<>&|\-+*\/%\^=]?=[^=]/ || - $tmpdata =~ /=[^= \\\n]/) { - print "Spacing around '=' or '==':\n"; - print "$file:$.: $line"; - $ret = 1; - } - - # One line conditional statements with one line bodies should - # not use curly brackets. - if ($data =~ /^\s*(if|while|for)\b.*\{$/) { - $cb_linenum = $.; - $cb_code = $line; - $cb_scolon = 0; - } - - # We need to check for exactly one semicolon inside the body, - # because empty statements (e.g. with comment only) are - # allowed - if ($cb_linenum == $. - 1 && $data =~ /^[^;]*;[^;]*$/) { - $cb_code .= $line; - $cb_scolon = 1; - } - - if ($data =~ /^\s*}\s*$/ && - $cb_linenum == $. - 2 && - $cb_scolon) { - - print "Curly brackets around single-line body:\n"; - print "$file:$cb_linenum-$.:\n$cb_code$line"; - $ret = 1; - - # There _should_ be no need to reset the values; but to - # keep my inner peace... - $cb_linenum = 0; - $cb_scolon = 0; - $cb_code = ""; - } - } - close FILE; -} - -exit $ret; diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl new file mode 100755 index 0000000..83b5898 --- /dev/null +++ b/build-aux/check-spacing.pl @@ -0,0 +1,204 @@ +#!/usr/bin/perl +# +# 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/>. +# +# Authors: +# Daniel P. Berrange <berrange@redhat.com> + +use strict; +use warnings; + +my $ret = 0; +my $incomment = 0; + +foreach my $file (@ARGV) { + # Per-file variables for multiline Curly Bracket (cb_) check + my $cb_linenum = 0; + my $cb_code = ""; + my $cb_scolon = 0; + + open FILE, $file; + + while (defined (my $line = <FILE>)) { + my $data = $line; + # For temporary modifications + my $tmpdata; + + # Kill any quoted , ; = or " + $data =~ s/'[";,=]'/'X'/g; + + # Kill any quoted strings + $data =~ s,"([^\\\"]|\\.)*","XXX",g; + + # Kill any C++ style comments + $data =~ s,//.*$,//,; + + next if $data =~ /^#/; + + # Kill contents of multi-line comments + # and detect end of multi-line comments + if ($incomment) { + if ($data =~ m,\*/,) { + $incomment = 0; + $data =~ s,^.*\*/,*/,; + } else { + $data = ""; + } + } + + # Kill single line comments, and detect + # start of multi-line comments + if ($data =~ m,/\*.*\*/,) { + $data =~ s,/\*.*\*/,/* */,; + } elsif ($data =~ m,/\*,) { + $incomment = 1; + $data =~ s,/\*.*,/*,; + } + + # We need to match things like + # + # int foo (int bar, bool wizz); + # foo (bar, wizz); + # + # but not match things like: + # + # typedef int (*foo)(bar wizz) + # + # we can't do this (efficiently) without + # missing things like + # + # foo (*bar, wizz); + # + # We also don't want to spoil the $data so it can be used + # later on. + $tmpdata = $data; + while ($tmpdata =~ /(\w+)\s\((?!\*)/) { + my $kw = $1; + + # Allow space after keywords only + if ($kw =~ /^(if|for|while|switch|return)$/) { + $tmpdata =~ s/($kw\s\()/XXX(/; + } else { + print "Whitespace after non-keyword:\n"; + print "$file:$.: $line"; + $ret = 1; + last; + } + } + + # Require whitespace immediately after keywords, + # but none after the opening bracket + if ($data =~ /\b(if|for|while|switch|return)\(/ || + $data =~ /\b(if|for|while|switch|return)\s+\(\s/) { + print "No whitespace after keyword:\n"; + print "$file:$.: $line"; + $ret = 1; + } + + # Forbid whitespace between )( of a function typedef + if ($data =~ /\(\*\w+\)\s+\(/) { + print "Whitespace between ')' and '(':\n"; + print "$file:$.: $line"; + $ret = 1; + } + + # Forbid whitespace following ( or prior to ) + if ($data =~ /\S\s+\)/ || + $data =~ /\(\s+\S/) { + print "Whitespace after '(' or before ')':\n"; + print "$file:$.: $line"; + $ret = 1; + } + + # Forbid whitespace before ";" or ",". Things like below are allowed: + # + # 1) The expression is empty for "for" loop. E.g. + # for (i = 0; ; i++) + # + # 2) An empty statement. E.g. + # while (write(statuswrite, &status, 1) == -1 && + # errno == EINTR) + # ; + # + if ($data =~ /[^;\s]\s+[;,]/) { + print "Whitespace before (semi)colon:\n"; + print "$file:$.: $line"; + $ret = 1; + } + + # Require EOL, macro line continuation, or whitespace after ";". + # Allow "for (;;)" as an exception. + if ($data =~ /;[^ \\\n;)]/) { + print "Invalid character after semicolon:\n"; + print "$file:$.: $line"; + $ret = 1; + } + + # Require EOL, space, or enum/struct end after comma. + if ($data =~ /,[^ \\\n)}]/) { + print "Invalid character after comma:\n"; + print "$file:$.: $line"; + $ret = 1; + } + + # Require spaces around assignment '=', compounds and '==' + # with the exception of virAssertCmpInt() + $tmpdata = $data; + $tmpdata =~ s/(virAssertCmpInt\(.* ).?=,/$1op,/; + if ($tmpdata =~ /[^ ]\b[!<>&|\-+*\/%\^=]?=[^=]/ || + $tmpdata =~ /=[^= \\\n]/) { + print "Spacing around '=' or '==':\n"; + print "$file:$.: $line"; + $ret = 1; + } + + # One line conditional statements with one line bodies should + # not use curly brackets. + if ($data =~ /^\s*(if|while|for)\b.*\{$/) { + $cb_linenum = $.; + $cb_code = $line; + $cb_scolon = 0; + } + + # We need to check for exactly one semicolon inside the body, + # because empty statements (e.g. with comment only) are + # allowed + if ($cb_linenum == $. - 1 && $data =~ /^[^;]*;[^;]*$/) { + $cb_code .= $line; + $cb_scolon = 1; + } + + if ($data =~ /^\s*}\s*$/ && + $cb_linenum == $. - 2 && + $cb_scolon) { + + print "Curly brackets around single-line body:\n"; + print "$file:$cb_linenum-$.:\n$cb_code$line"; + $ret = 1; + + # There _should_ be no need to reset the values; but to + # keep my inner peace... + $cb_linenum = 0; + $cb_scolon = 0; + $cb_code = ""; + } + } + close FILE; +} + +exit $ret; diff --git a/cfg.mk b/cfg.mk index 2939458..e7d49b6 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1104,12 +1104,12 @@ _autogen: # regenerate HACKING as part of the syntax-check ifneq ($(_gl-Makefile),) -syntax-check: $(top_srcdir)/HACKING bracket-spacing-check test-wrap-argv +syntax-check: $(top_srcdir)/HACKING spacing-check test-wrap-argv endif -bracket-spacing-check: +spacing-check: $(AM_V_GEN)files=`$(VC_LIST) | grep '\.c$$'`; \ - $(PERL) $(top_srcdir)/build-aux/bracket-spacing.pl $$files || \ + $(PERL) $(top_srcdir)/build-aux/check-spacing.pl $$files || \ { echo '$(ME): incorrect formatting, see HACKING for rules' 1>&2; \ exit 1; } -- 2.7.3

On Wed, Jun 15, 2016 at 12:06:53 +0200, Ján Tomko wrote:
We test whitespace with lots of other characters now. --- build-aux/bracket-spacing.pl | 204 ------------------------------------------- build-aux/check-spacing.pl | 204 +++++++++++++++++++++++++++++++++++++++++++ cfg.mk | 6 +- 3 files changed, 207 insertions(+), 207 deletions(-) delete mode 100755 build-aux/bracket-spacing.pl create mode 100755 build-aux/check-spacing.pl
ACK

Instead of matching multiple characters, match any occurrence preceded by a single whitespace and handle the exceptions later. --- build-aux/check-spacing.pl | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl index 83b5898..d693fbe 100755 --- a/build-aux/check-spacing.pl +++ b/build-aux/check-spacing.pl @@ -135,10 +135,13 @@ foreach my $file (@ARGV) { # errno == EINTR) # ; # - if ($data =~ /[^;\s]\s+[;,]/) { - print "Whitespace before (semi)colon:\n"; - print "$file:$.: $line"; - $ret = 1; + if ($data =~ /\s[;,]/) { + unless ($data =~ /\S; ; / || + $data =~ /^\s+;/) { + print "Whitespace before (semi)colon:\n"; + print "$file:$.: $line"; + $ret = 1; + } } # Require EOL, macro line continuation, or whitespace after ";". -- 2.7.3

On Wed, Jun 15, 2016 at 12:06:54 +0200, Ján Tomko wrote:
Instead of matching multiple characters, match any occurrence preceded by a single whitespace and handle the exceptions later. --- build-aux/check-spacing.pl | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl index 83b5898..d693fbe 100755 --- a/build-aux/check-spacing.pl +++ b/build-aux/check-spacing.pl @@ -135,10 +135,13 @@ foreach my $file (@ARGV) { # errno == EINTR) # ; # - if ($data =~ /[^;\s]\s+[;,]/) { - print "Whitespace before (semi)colon:\n"; - print "$file:$.: $line"; - $ret = 1; + if ($data =~ /\s[;,]/) { + unless ($data =~ /\S; ; / || + $data =~ /^\s+;/) { + print "Whitespace before (semi)colon:\n";
This check disallows whitespace before commas and semicolons but not colons. ACK

Instead of matching multiple characters before the parenthesis, only check for a single whitespace, which is much less cpu-intensive. This only matches a few dozen of places where they are on an separate line, filter out those with a separate regex. --- build-aux/check-spacing.pl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl index d693fbe..962eabb 100755 --- a/build-aux/check-spacing.pl +++ b/build-aux/check-spacing.pl @@ -118,7 +118,9 @@ foreach my $file (@ARGV) { } # Forbid whitespace following ( or prior to ) - if ($data =~ /\S\s+\)/ || + # but allow whitespace before ) on a single line + # (optionally followed by a semicolon) + if (($data =~ /\s\)/ && not $data =~ /^\s+\);?\s*$/) || $data =~ /\(\s+\S/) { print "Whitespace after '(' or before ')':\n"; print "$file:$.: $line"; -- 2.7.3

On Wed, Jun 15, 2016 at 12:06:55 +0200, Ján Tomko wrote:
Instead of matching multiple characters before the parenthesis, only check for a single whitespace, which is much less cpu-intensive.
This only matches a few dozen of places where they are on an separate line, filter out those with a separate regex. --- build-aux/check-spacing.pl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl index d693fbe..962eabb 100755 --- a/build-aux/check-spacing.pl +++ b/build-aux/check-spacing.pl @@ -118,7 +118,9 @@ foreach my $file (@ARGV) { }
# Forbid whitespace following ( or prior to ) - if ($data =~ /\S\s+\)/ || + # but allow whitespace before ) on a single line + # (optionally followed by a semicolon) + if (($data =~ /\s\)/ && not $data =~ /^\s+\);?\s*$/) ||
As we have a separate check for 'trailing_blank' I don't think you need to add the '\s*' part at the end. ACK

We do not need a separate check forbidding whitespace after the opening parenthesis after a keyword - we forbid it after all of them. The only allowed whitespace after an opening parenthesis is a newline, tune the regex to reflect that. --- build-aux/check-spacing.pl | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl index 962eabb..044cd25 100755 --- a/build-aux/check-spacing.pl +++ b/build-aux/check-spacing.pl @@ -101,10 +101,8 @@ foreach my $file (@ARGV) { } } - # Require whitespace immediately after keywords, - # but none after the opening bracket - if ($data =~ /\b(if|for|while|switch|return)\(/ || - $data =~ /\b(if|for|while|switch|return)\s+\(\s/) { + # Require whitespace immediately after keywords + if ($data =~ /\b(if|for|while|switch|return)\(/) { print "No whitespace after keyword:\n"; print "$file:$.: $line"; $ret = 1; @@ -121,7 +119,7 @@ foreach my $file (@ARGV) { # but allow whitespace before ) on a single line # (optionally followed by a semicolon) if (($data =~ /\s\)/ && not $data =~ /^\s+\);?\s*$/) || - $data =~ /\(\s+\S/) { + $data =~ /\((?!$)\s/) { print "Whitespace after '(' or before ')':\n"; print "$file:$.: $line"; $ret = 1; -- 2.7.3

On Wed, Jun 15, 2016 at 12:06:56 +0200, Ján Tomko wrote:
We do not need a separate check forbidding whitespace after the opening parenthesis after a keyword - we forbid it after all of them.
The only allowed whitespace after an opening parenthesis is a newline, tune the regex to reflect that. --- build-aux/check-spacing.pl | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
ACK

The macro is now called testAssertEq and no longer takes an operator as its argument. --- build-aux/check-spacing.pl | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl index 044cd25..f66ae10 100755 --- a/build-aux/check-spacing.pl +++ b/build-aux/check-spacing.pl @@ -160,11 +160,8 @@ foreach my $file (@ARGV) { } # Require spaces around assignment '=', compounds and '==' - # with the exception of virAssertCmpInt() - $tmpdata = $data; - $tmpdata =~ s/(virAssertCmpInt\(.* ).?=,/$1op,/; - if ($tmpdata =~ /[^ ]\b[!<>&|\-+*\/%\^=]?=[^=]/ || - $tmpdata =~ /=[^= \\\n]/) { + if ($data =~ /[^ ]\b[!<>&|\-+*\/%\^=]?=/ || + $data =~ /=[^= \\\n]/) { print "Spacing around '=' or '==':\n"; print "$file:$.: $line"; $ret = 1; -- 2.7.3

On Wed, Jun 15, 2016 at 12:06:57 +0200, Ján Tomko wrote:
The macro is now called testAssertEq and no longer takes an operator as its argument. --- build-aux/check-spacing.pl | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
ACK

--- build-aux/check-spacing.pl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/build-aux/check-spacing.pl b/build-aux/check-spacing.pl index f66ae10..55687f2 100755 --- a/build-aux/check-spacing.pl +++ b/build-aux/check-spacing.pl @@ -43,7 +43,7 @@ foreach my $file (@ARGV) { $data =~ s/'[";,=]'/'X'/g; # Kill any quoted strings - $data =~ s,"([^\\\"]|\\.)*","XXX",g; + $data =~ s,"(?:[^\\\"]|\\.)*","XXX",g; # Kill any C++ style comments $data =~ s,//.*$,//,; @@ -91,8 +91,8 @@ foreach my $file (@ARGV) { my $kw = $1; # Allow space after keywords only - if ($kw =~ /^(if|for|while|switch|return)$/) { - $tmpdata =~ s/($kw\s\()/XXX(/; + if ($kw =~ /^(?:if|for|while|switch|return)$/) { + $tmpdata =~ s/(?:$kw\s\()/XXX(/; } else { print "Whitespace after non-keyword:\n"; print "$file:$.: $line"; @@ -102,7 +102,7 @@ foreach my $file (@ARGV) { } # Require whitespace immediately after keywords - if ($data =~ /\b(if|for|while|switch|return)\(/) { + if ($data =~ /\b(?:if|for|while|switch|return)\(/) { print "No whitespace after keyword:\n"; print "$file:$.: $line"; $ret = 1; -- 2.7.3
participants (2)
-
Ján Tomko
-
Peter Krempa