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

[Brandon Williams <bmwill@google.com>]

On 08/08, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> writes:
> 
> > 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.

This is an issue when you have an inconsistent code base such as ours as
the tool, even when used to format a diff, will format the surrounding
context.

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

I would hope that use of such a tool would eventually completely
eliminate style-only critiques.  

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

I know that when you introduce a formatter there are bound to be some
issues where a human would choose one way over the other.  If we start
using a formatter I would expect some of the penalties would need to be
tweaked overtime before we rely too heavily on it.

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

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

Well there's no 'C' Lang option so Cpp is the closest there is ;)

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

Haha So generally no we don't do this.  Though there are definitely many
places in our code base where we do.  Personally this makes it a bit
easier to read when you end up having long function names.  I also
worked on a code base which did this and it made it incredible easy to
grep for the definition of a function.  If you grep for 'foo()' then
you'd get all the uses of the function including the definition but if
you grep for '^foo()' you'd get only the definition.

But that's my preference, if we end up using this tool it would probably
make sense to change this.

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

The alternative (with this config) is to have arguments and parameters
each on their own separate line.

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

I've seen both and I wasn't sure which was the correct form to use.

> 
> Thanks.

-- 
Brandon Williams