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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 07, 2014 at 09:58:11AM -0500, Laine Stump wrote:
On 11/07/2014 05:09 AM, Eric Blake wrote:
On 11/06/2014 05:38 PM, Martin Kletzander wrote:

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

I've always thought this, and used to do it that way until a few
reviewers complained. So I for one would welcome our new "use braces if
condition is multiple lines" overlords :-)

(I also agree that this is all an exercise in futility unless there is a
matching syntax-check rule to enforce it.)

Well, I used to place opening curly brackets on their own new lines as
we do with functions (pre-libvirt) and this is something I had to get
used to.

Anyway, I'd be more than OK with changing contributor guidelines and
saying that we require curly brackets if either a) the body or b) the
condition itself spans over multiple lines.  syntax-check for that is
*way* easier ;)

Martin

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]