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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> Yeah. I obviously was adapting the original text, and I think I left too
> much of the wishy-washy-ness in. As the point of the patch is to avoid
> that, let's take your suggestion. A re-rolled patch is below.
>
> Now the patch is at least self-consistent. The bigger question remains
> of: do we want to dictate these rules, or did we prefer the vague
> version?

I would say no to make all of them "rules to be dictated".  It is OK
to leave some things "gray".  

But obviously others can vote differently ;-)

> I _thought_ the vague rules were doing fine, but this whole discussion
> has made me think otherwise. And I'd just as soon not ever have to
> repeat it. ;-/
>
> OTOH, I really do not want to review a bunch of patches that do nothing
> but change brace style (and I am sure there is a mix of styles already
> in the code base).

Yes, exactly, that is why the last sentence is in there below.

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

> The existing document says:
>
>     Make your code readable and sensible, and don't try to be clever.
>     
>     As for more concrete guidelines, just imitate the existing code
>     (this is a good guideline, no matter which project you are
>     contributing to). It is always preferable to match the _local_
>     convention. New code added to Git suite is expected to match
>     the overall style of existing code. Modifications to existing
>     code is expected to match the style the surrounding code already
>     uses (even if it doesn't match the overall style of existing code).
>     
>     But if you must have a list of rules, here they are.
>
> I guess that is the place to expound on how to interpret the rules. I
> dunno. Some of the individual rules that go into "gray areas" already
> spell out the "surrounding code" rule.


We are not saying one important thing and that was what invited
useless arguing this time: For "gray" things, the choice is yours in
your new code, but do not churn existing code for "gray" guidelines.

If we made _that_ clear as a rule we dictate, hopefully we'd have
less chance to see "a bunch of patches that do nothing but change
brace style", I would think.

> -- >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..a4191aa38 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 and some of them
> +	  require braces, enclose even a single line block in braces for
> +	  consistency. E.g.:
> +
> +		if (foo) {
> +			doit();
> +		} else {
> +			one();
> +			two();
> +			three();
> +		}
>  
>   - We try to avoid assignments in the condition of an "if" statement.