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