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

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(a)redhat.com&gt; > > --- > > > +++ 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