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@xxxxxxxxxx> > --- > +++ 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list