git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
BWBrandon Williams <bmwill@google.com>
Date
Aug 8, 2017, 18:03 UTC
Message-ID
<20170808180317.GA73298@google.com>
In-Reply-To
<xmqq3792c5sb.fsf@gitster.mtv.corp.google.com>
On 08/08, Junio C Hamano wrote:
Show 34 quoted lines
> 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.

Show 7 quoted lines
> 
>  - 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.

Show 6 quoted lines
> 
>  - 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.
Show 48 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 ;-)
Well there's no 'C' Lang option so Cpp is the closest there is ;)
Show 7 quoted lines
> 
> > +# 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.

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

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

Show 7 quoted lines
> 
> > +# 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
Previous: Junio C HamanoNext: Junio C Hamano
Message 17 of 64 in “[RFC] clang-format: outline the git project's coding style”
  1. Brandon WilliamsAug 8, 2017
  2. Johannes SchindelinAug 8, 2017
  3. Stefan BellerAug 8, 2017
  4. Brandon WilliamsAug 8, 2017
  5. Johannes SchindelinAug 9, 2017
  6. Stefan BellerAug 9, 2017
  7. Johannes SchindelinAug 10, 2017
  8. Junio C HamanoAug 10, 2017
  9. Brandon WilliamsAug 10, 2017
  10. Junio C HamanoAug 10, 2017
  11. Brandon WilliamsAug 10, 2017
  12. Ben PeartAug 11, 2017
  13. Johannes SchindelinAug 14, 2017
  14. Johannes SchindelinSep 28, 2017
  15. Brandon WilliamsSep 28, 2017
  16. Junio C HamanoAug 8, 2017
  17. Brandon WilliamsAug 8, 2017
  18. Junio C HamanoAug 8, 2017
  19. Junio C HamanoAug 9, 2017
  20. Brandon WilliamsAug 11, 2017
  21. Junio C HamanoAug 11, 2017
  22. Jeff KingAug 9, 2017
  23. Ævar Arnfjörð BjarmasonAug 9, 2017
  24. Stefan BellerAug 9, 2017
  25. Stefan BellerAug 9, 2017
  26. Jeff KingAug 9, 2017
  27. brian m. carlsonAug 15, 2017
  28. Jonathan NiederAug 15, 2017
  29. Brandon WilliamsAug 11, 2017
  30. Jeff KingAug 11, 2017
  31. Junio C HamanoAug 12, 2017
  32. Jeff KingAug 13, 2017
  33. Ramsay JonesAug 13, 2017
  34. Ramsay JonesAug 13, 2017
  35. Junio C HamanoAug 13, 2017
  36. Ramsay JonesAug 13, 2017
  37. 0/2 clang-formatBrandon Williams, Aug 14, 2017
  38. 2/2 Makefile: add style build ruleBrandon Williams, Aug 14, 2017
  39. Stefan BellerAug 14, 2017
  40. Junio C HamanoAug 14, 2017
  41. Stefan BellerAug 14, 2017
  42. Jeff KingAug 14, 2017
  43. Junio C HamanoAug 14, 2017
  44. Junio C HamanoAug 14, 2017
  45. Brandon WilliamsAug 15, 2017
  46. Jeff KingAug 15, 2017
  47. 1/2 clang-format: outline the git project's coding styleBrandon Williams, Aug 14, 2017
  48. Stefan BellerAug 14, 2017
  49. Ben PeartAug 15, 2017
  50. Brandon WilliamsAug 15, 2017
  51. Jeff KingAug 14, 2017
  52. Jeff KingAug 14, 2017
  53. Brandon WilliamsAug 14, 2017
  54. Jeff KingAug 14, 2017
  55. Johannes SchindelinAug 16, 2017
  56. Ben PeartAug 15, 2017
  57. Brandon WilliamsAug 15, 2017
  58. Stefan BellerAug 15, 2017
  59. Brandon WilliamsAug 14, 2017
  60. Jeff KingAug 14, 2017
  61. Stefan BellerAug 14, 2017
  62. Jeff KingAug 15, 2017
  63. Junio C HamanoAug 15, 2017
  64. Jeff KingAug 15, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.