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
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list