Re: [RFC] clang-format: outline the git project's coding style

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 8, 2017, 17:45 UTC
Message-ID: <xmqq3792c5sb.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170808012554.186051-1-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

Show 20 quoted lines

> Add a '.clang-format' file which outlines the git project's coding
> style.  This can be used with clang-format to auto-format .c and .h
> files to conform with git's style.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
>
> I'm sure this sort of thing comes up every so often on the list but back at
> git-merge I mentioned how it would be nice to not have to worry about style
> when reviewing patches as that is something mechanical and best left to a
> machine (for the most part).  I saw that 'clang-format' was brought up on the
> list once before a couple years ago
> (https://public-inbox.org/git/20150121220903.GA10267@peff.net/) but nothing
> really came of it.  I spent a little bit of time combing through the various
> options and came up with this config based on the general style of our code
> base.  The big issue though is that our code base isn't consistent so try as
> you might you wont be able to come up with a config which matches everything we
> do (mostly due to the inconsistencies in our code base).
>
> Anyway, I thought I'd bring this topic back up and see how people feel about it.

I gave just one pass over all the rules you have here. I didn't think too deeply about implications of some of them, but most of them looked sensible. Thanks for compiling this set of rules.

Having said that, it is easier to refine individual rules you have below than to make sure that among the develoepers there is a shared notion of how these rules are to be used. If we get that part wrong, we'd see unpleasant consequences, e.g.

 - We may see unwanted code churn on existing codebase, only for the
   sake of satisfying the formatting rules specified here.

 - We may see far more style-only critique to patches posted on the
   list simply because there are more readers than writers, and it
   is likely that running the tool to nitpick other people's patches
   is far easier than writing these patches in the first place (and
   forgetting to ask the formatter to nitpick before sending them
   out).

 - Human aesthetics judgement often is necessary to overrule
   mechanical rules (e.g. A human may have two pairs of <ptr, len>
   parameters on separate lines in a function declaration;
   BinPackParameters may try to break after ptrA, lenA, ptrB before
   lenB in such a case).

We need to set our expectation and a guideline to apply these rules straight, before introducing something like this.

Show 19 quoted lines

>  .clang-format | 166 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 166 insertions(+)
>  create mode 100644 .clang-format
>
> diff --git a/.clang-format b/.clang-format
> new file mode 100644
> index 000000000..7f28dc259
> --- /dev/null
> +++ b/.clang-format
> @@ -0,0 +1,166 @@
> +# Defaults
> +
> +# Use tabs whenever we need to fill whitespace that spans at least from one tab
> +# stop to the next one.
> +UseTab: Always
> +TabWidth: 8
> +IndentWidth: 8
> +ContinuationIndentWidth: 8
> +ColumnLimit: 80

I often deliberately chomp my lines much shorter than this limit, and also I deliberately write a line that is a tad longer than this limit some other times, if doing so makes the result easier to read. And I know other develoepers with good taste do the same. It is pointless to have a discussion that begins with "who uses a physical terminal these days that can only show 80-columns?" to tweak this value, I think. It is more important to give a guideline on what to do when lines in your code goes over this limit.

A mechanical "formatter" would just find an appropriate point in a line with least "penalty" and chomp it into two. But an appropriate way to make such a code that is way too deeply indented readable may instead be judicious use of goto's and one-time helper functions, for example, which mechanical tools would not do.

That is an example of what I meant above, i.e. a guideline to apply these rules. We would not want to say "clang-format suggests this rewrite, so we should accept the resulting code that is still too deeply indented as-is"---using the tool to point out an issue is good, though.

> +
> +# C Language specifics
> +Language: Cpp

Hmph ;-)

> +# Add a line break after the return type of top-level functions
> +# int
> +# foo();
> +AlwaysBreakAfterReturnType: TopLevel

We do that?

Show 5 quoted lines

> +# Pack as many parameters or arguments onto the same line as possible
> +# int myFunction(int aaaaaaaaaaaa, int bbbbbbbb,
> +#                int cccc);
> +BinPackArguments: true
> +BinPackParameters: true

I am OK with this but with the caveats I already mentioned.

> +# Insert a space after a cast
> +# x = (int32) y;    not    x = (int32)y;
> +SpaceAfterCStyleCast: true

Hmph, I thought we did the latter, i.e. cast sticks to the casted expression without SP.

Thanks.