On 11/06/2014 05:38 PM, Martin Kletzander wrote:
> As stated in our contributor guidelines, we don't want curly brackets
> around oneline code block (with some exceptions).
>
> Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> ---
> +++ b/src/conf/capabilities.c
> @@ -219,9 +219,8 @@ virCapabilitiesDispose(void *object)
> VIR_FREE(caps->host.migrateTrans[i]);
> VIR_FREE(caps->host.migrateTrans);
>
> - for (i = 0; i < caps->host.nsecModels; i++) {
> + for (i = 0; i < caps->host.nsecModels; i++)
> virCapabilitiesClearSecModel(&caps->host.secModels[i]);
> - }
Conversions like this make sense...
> @@ -138,9 +138,9 @@ virDevicePCIAddressEqual(virDevicePCIAddress *addr1,
> if (addr1->domain == addr2->domain &&
> addr1->bus == addr2->bus &&
> addr1->slot == addr2->slot &&
> - addr1->function == addr2->function) {
> + addr1->function == addr2->function)
> return true;
> - }
Conversions like this are a little harder to read (that is, any time the
'if' condition extends over multiple lines, but there is exactly four
space indentation of the condition, it's visually hard to see where the
multi-line condition ends and the loop body begins). The opening { is
hard to see either way, but seeing the closing } is my visual clue that
"yes, the last line of this blob of code is not part of the 'if' but the
one-line body". I'm not opposed to your removal of {}, but wonder if we
should rethink our style to recommend keeping the {} if the 'if'
expression is multiline.
Besides, it is easier to write a syntax check that checks for one-line
'if' expressions (that's how I wrote my checks for mis-matched if{}else;
and if;else{}) than it is for multi-line if expressions. So
distinguishing between the two types may make it easier to write a
syntax check and enforce a consistent style.
> @@ -3762,23 +3759,22 @@ virDomainDeviceInfoParseXML(xmlNodePtr node,
> if (cur->type == XML_ELEMENT_NODE) {
> if (alias == NULL &&
> !(flags & VIR_DOMAIN_XML_INACTIVE) &&
> - xmlStrEqual(cur->name, BAD_CAST "alias")) {
> + xmlStrEqual(cur->name, BAD_CAST "alias"))
> alias = cur;
This one is multi-line with no indentation to help...
> - } else if (address == NULL &&
> - xmlStrEqual(cur->name, BAD_CAST "address")) {
> + else if (address == NULL &&
> + xmlStrEqual(cur->name, BAD_CAST "address"))
> address = cur;
...while this one has indentation help because of the 'else if'; see the
difference in spotting the one-line statement?
> @@ -6879,9 +6870,8 @@ virDomainActualNetDefParseXML(xmlNodePtr node,
> if (!(actual->virtPortProfile
> = virNetDevVPortProfileParse(virtPortNode,
> VIR_VPORT_XML_REQUIRE_ALL_ATTRIBUTES |
> - VIR_VPORT_XML_REQUIRE_TYPE))) {
> + VIR_VPORT_XML_REQUIRE_TYPE)))
> goto error;
> - }
And this is an example of a multi-line 'if' expression, but it is
indented (both because the '=' is split over lines, and because the last
part of the expression is a function call split over lines), so the
one-liner is easy to spot here.
But my earlier claim that one-liner 'if' expressions are easier to grep
for than multi-line 'if' may mean that a syntax check would not flag
this one as a place to remove the {}. Maybe it doesn't matter.
(Ideally, if we could figure out how to make GNU indent or some other
code prettifier enforce a style, we'd use that instead of syntax check
grep rules).
So, I'm welcome to other opinions...
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list