Re: [PATCH] CodingGuidelines: clarify multi-line brace style

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 17, 2017, 19:39 UTC
Message-ID: <xmqqfukhfpcc.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170116220821.4tji5mrfcdbdpfuo@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 8 quoted lines

>> I think this is pretty clearly the "gray area" mentioned there. Which
>> yes, does not say "definitely do it this way", but I hope makes it clear
>> that you're supposed to use judgement about readability.
>
> So here's a patch.
>
> I know we've usually tried to keep this file to guidelines and not
> rules, but clearly it has not been clear-cut enough in this instance.

I have two "Huh?" with this patch.

 1. Two exceptions are not treated the same way.  The first one is
    "... extends over a few lines, it is excempt from the rule,
    period".  The second one, is ambivalent by saying "it can make
    sense", implying that "it may not make sense", so I am not sure
    if this is clarifying that much.

    If we want to clarify, perhaps drop "it can make sense to" and
    say

	When there are multiple arms to a conditional, and some of
	them require braces, enclose even a single line block in
	braces for consistency.

    perhaps?

 2. I actually think it is OK to leave some things "gray", but the
    confusion comes when people do not know what to do to things
    that are "gray", and they need a rule for that to be spelled
    out.

	When the project says it does not have strong preference
	among multiple choices, you are welcome to write your new
	code using any of them, as long as you are consistent with
	surrounding code.  Do not change style of existing code only
	to flip among these styles, though.

    That obviously is not limited to the rule/guideline for braces.

Show 60 quoted lines

> -- >8 --
> Subject: [PATCH] CodingGuidelines: clarify multi-line brace style
>
> There are some "gray areas" around when to omit braces from
> a conditional or loop body. Since that seems to have
> resulted in some arguments, let's be a little more clear
> about our preferred style.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  Documentation/CodingGuidelines | 37 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 4cd95da6b..0e336e99d 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -206,11 +206,38 @@ For C programs:
>  		x = 1;
>  	}
>  
> -   is frowned upon.  A gray area is when the statement extends
> -   over a few lines, and/or you have a lengthy comment atop of
> -   it.  Also, like in the Linux kernel, if there is a long list
> -   of "else if" statements, it can make sense to add braces to
> -   single line blocks.
> +   is frowned upon. But there are a few exceptions:
> +
> +	- When the statement extends over a few lines (e.g., a while loop
> +	  with an embedded conditional, or a comment). E.g.:
> +
> +		while (foo) {
> +			if (x)
> +				one();
> +			else
> +				two();
> +		}
> +
> +		if (foo) {
> +			/*
> +			 * This one requires some explanation,
> +			 * so we're better off with braces to make
> +			 * it obvious that the indentation is correct.
> +			 */
> +			doit();
> +		}
> +
> +	- When there are multiple arms to a conditional, it can make
> +	  sense to add braces to single line blocks for consistency.
> +	  E.g.:
> +
> +		if (foo) {
> +			doit();
> +		} else {
> +			one();
> +			two();
> +			three();
> +		}
>  
>   - We try to avoid assignments in the condition of an "if" statement.