On Mon, 2018-09-03 at 17:13 +0200, Michal Privoznik wrote: > On 08/31/2018 07:35 PM, John Ferlan wrote: > > > + if (!(ret->lockPlugin = virLockManagerPluginNew(lockManagerName, > > > + NULL, NULL, 0))) { > > > + goto error; > > > + } > > > > The { } aren't necessary, but understandable. > > Andrea would disagree. In one other patch that I had on the list > recently he made me to put the brackets there arguing that if() is not a > single line or something. The relevant chapter of our guidelines[1] states: Omit the curly braces around an if, while, for etc. body only when both that body and the condition itself occupy a single line. In every other case we require the braces. This ensures that it is trivially easy to identify a single-statement loop: each has only one line in its body. while (expr) // single line body; {} is forbidden single_line_stmt(); while (expr(arg1, arg2)) // indentation makes it obvious it is single line, single_line_stmt(); // {} is optional (not enforced either way) while (expr1 && expr2) { // multi-line, at same indentation, {} required single_line_stmt(); } In the recent patch you mention[2] we were squarely in Example Three territory, so there was no question about whether or not curly braces should be added. This time around we may refer to Example Two, "not enforced either way", but given just how much whitespace there is on the second line I would argue the braces should definitely be there to disambiguate the situation. > On a side note, this points to a bug in our coding style. Either we > should put brackets everywhere (even for true single line if()-s like > those in the context above), or not be picky about "multiline" if()-s > like the one we are talking about here. Completely agree. We have way too many subtleties in our guidelines, which leads to uneven adoption despite syntax-check and people like me annoying contributors during review :) While switching to an all braces, all the time approach is probably not realistic due to the sheer amount of changes that would be required to make existing code compliant, we could start requiring it for new code and go from there. [1] https://libvirt.org/hacking.html#curly_braces [2] https://www.redhat.com/archives/libvir-list/2018-August/msg01269.html -- Andrea Bolognani / Red Hat / Virtualization -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list