Re: [PATCH] CodingGuidelines: clarify multi-line brace style
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 17, 2017, 21:41 UTC
- Message-ID
- <xmqqshohcqkv.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170117200503.3iwaedrgfq544b4i@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> 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 ;-)
Show 7 quoted lines
> 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.
Show 5 quoted lines
>> 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.
Show 17 quoted lines
> 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.
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..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.