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

[Daniel P. Berrange]

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


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|