[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 02/15] Remove unneeded curly brackets around oneline code blocks in src/conf/

On Fri, Nov 07, 2014 at 11:11:27AM +0100, Daniel P. Berrange wrote:
On Fri, Nov 07, 2014 at 11:09:11AM +0100, Eric Blake wrote:
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 redhat com>
> ---

> +++ 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).

Yeah, I'd really like to be able to run something like GNU indent over
the code and check input==output, but there were a couple of rules in
our coding style that indent didn't appear to support

I tried playing with the indent tool many times but I've never got to
any better situation than where we were at the time I started hacking
on libvirt.  Maybe if we can adjust our rules to match whatever indent
supports, although I don't remember what the particular problems were.


Attachment: signature.asc
Description: Digital signature

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]