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

[Martin Kletzander]

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 at 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.

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20141107/a3b1d45a/attachment-0001.sig>