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

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