Re: [RFC] clang-format: outline the git project's coding style
- Date: Tue, 8 Aug 2017 11:03:17 -0700
- From: Brandon Williams <bmwill@xxxxxxxxxx>
- Subject: Re: [RFC] clang-format: outline the git project's coding style
On 08/08, Junio C Hamano wrote:
> Brandon Williams <bmwill@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxx/) 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
> - 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
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.