{"thread":{"id":"38394","subject":"[PATCH] .clang-format: introduce the use of clang-format","startedAt":"2015-01-17T21:30:21Z","lastAt":"2015-01-21T22:09:03Z","messageCount":7,"participants":["Ramkumar Ramachandra","René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"254858","messageId":"1421530221-39306-1-git-send-email-artagnon@gmail.com","threadId":"38394","inReplyTo":null,"subject":"[PATCH] .clang-format: introduce the use of clang-format","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2015-01-17T21:30:21Z","receivedAt":"2015-01-17T21:30:21Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Instead of manually eyeballing style in reviews, just ask all\ncontributors to run their patches through [git-]clang-format.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n The idea is to introduce the community to this new toy I found called\n clang-format. Whether or not it's actually going to be used doesn't\n bother me too much.\n\n I'm not 100% sure of the style, but I'll leave you to tweak that\n using http://clang.llvm.org/docs/ClangFormatStyleOptions.html\n\n The current code isn't terribly conformant, but I suppose that'll\n change with time.\n\n .clang-format | 7 +++++++\n 1 file changed, 7 insertions(+)\n create mode 100644 .clang-format\n\ndiff --git a/.clang-format b/.clang-format\nnew file mode 100644\nindex 0000000..63a53e0\n--- /dev/null\n+++ b/.clang-format\n@@ -0,0 +1,7 @@\n+BasedOnStyle: LLVM\n+IndentWidth: 8\n+UseTab: Always\n+BreakBeforeBraces: Linux\n+AllowShortBlocksOnASingleLine: false\n+AllowShortIfStatementsOnASingleLine: false\n+IndentCaseLabels: false\n\\ No newline at end of file\n-- \n2.2.1\n"},{"id":"254861","messageId":"54BB9986.2040706@web.de","threadId":"38394","inReplyTo":"1421530221-39306-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH] .clang-format: introduce the use of clang-format","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2015-01-18T11:31:18Z","receivedAt":"2015-01-18T11:31:18Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.01.2015 um 22:30 schrieb Ramkumar Ramachandra:\n> Instead of manually eyeballing style in reviews, just ask all\n> contributors to run their patches through [git-]clang-format.\n>\n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n> ---\n>   The idea is to introduce the community to this new toy I found called\n>   clang-format. Whether or not it's actually going to be used doesn't\n>   bother me too much.\n>\n>   I'm not 100% sure of the style, but I'll leave you to tweak that\n>   using http://clang.llvm.org/docs/ClangFormatStyleOptions.html\n>\n>   The current code isn't terribly conformant, but I suppose that'll\n>   change with time.\n>\n>   .clang-format | 7 +++++++\n>   1 file changed, 7 insertions(+)\n>   create mode 100644 .clang-format\n>\n> diff --git a/.clang-format b/.clang-format\n> new file mode 100644\n> index 0000000..63a53e0\n> --- /dev/null\n> +++ b/.clang-format\n> @@ -0,0 +1,7 @@\n> +BasedOnStyle: LLVM\n> +IndentWidth: 8\n> +UseTab: Always\n> +BreakBeforeBraces: Linux\n> +AllowShortBlocksOnASingleLine: false\n> +AllowShortIfStatementsOnASingleLine: false\n> +IndentCaseLabels: false\n> \\ No newline at end of file\n\nWhy no newline on the last line?\n\nThese one would be needed as well to match our style, I think:\n\n\tAllowShortFunctionsOnASingleLine: None\n\tContinuationIndentWidth: 8\n\nAnd probably this one:\n\n\tCpp11BracedListStyle: false\n\nHowever, even then struct declarations that are combined with variable \ndeclaration and initialization get mangled:\n\n\tstruct a {\n\t\tint n;\n\t\tconst char *s;\n\t} arr[] = {\n\t\t{ 1, \"one\" },\n\t\t{ 2, \"two\" }\n\t};\n\nbecomes:\n\n\tstruct a\n\t{\n\t\tint n;\n\t\tconst char *s;\n\t} arr[] = { { 1, \"one\" }, { 2, \"two\" } };\n\nIt gets formatted better if arr is declared separately.\n\nAnd this one helps get rid of the added line break between struct a and \nthe following brace:\n\n\tBreakBeforeBraces: Stroustrup\n"},{"id":"254993","messageId":"1421859687-27216-1-git-send-email-artagnon@gmail.com","threadId":"38394","inReplyTo":"1421530221-39306-1-git-send-email-artagnon@gmail.com","subject":"[PATCH v2] .clang-format: introduce the use of clang-format","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2015-01-21T17:01:27Z","receivedAt":"2015-01-21T17:01:27Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Instead of manually eyeballing style in reviews, just ask all\ncontributors to run their patches through [git-]clang-format.\n\nHowever, struct declarations that are combined with variable\ndeclaration and initialization get mangled:\n\n        struct a {\n                int n;\n                const char *s;\n        } arr[] = {\n                { 1, \"one\" },\n                { 2, \"two\" }\n        };\n\nbecomes:\n\n        struct a {\n                int n;\n                const char *s;\n        } arr[] = { { 1, \"one\" }, { 2, \"two\" } };\n\nIt gets formatted better if arr is declared separately.\n\nHelped-by: René Scharfe <l.s.r@web.de>\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n .clang-format | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n create mode 100644 .clang-format\n\ndiff --git a/.clang-format b/.clang-format\nnew file mode 100644\nindex 0000000..a336438\n--- /dev/null\n+++ b/.clang-format\n@@ -0,0 +1,11 @@\n+BasedOnStyle: LLVM\n+IndentWidth: 8\n+UseTab: Always\n+BreakBeforeBraces: Linux\n+AllowShortBlocksOnASingleLine: false\n+AllowShortIfStatementsOnASingleLine: false\n+IndentCaseLabels: false\n+AllowShortFunctionsOnASingleLine: None\n+ContinuationIndentWidth: 8\n+Cpp11BracedListStyle: false\n+BreakBeforeBraces: Stroustrup\n-- \n2.2.1\n"},{"id":"254994","messageId":"CALkWK0n9MCxhTyRaJgXRSeO02A-uRvW8Ft9Ns8oxXzctpEK66w@mail.gmail.com","threadId":"38394","inReplyTo":"54BB9986.2040706@web.de","subject":"Re: [PATCH] .clang-format: introduce the use of clang-format","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2015-01-21T17:06:12Z","receivedAt":"2015-01-21T17:06:12Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"René Scharfe wrote:\n> However, even then struct declarations that are combined with variable\n> declaration and initialization get mangled:\n\nI'm pretty sure this is a bug in clang-format. It might be\nsemi-trivial to fix too.\n\nThanks for your inputs.\n"},{"id":"255029","messageId":"20150121204502.GA3287@peff.net","threadId":"38394","inReplyTo":"1421859687-27216-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH v2] .clang-format: introduce the use of clang-format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-21T20:45:02Z","receivedAt":"2015-01-21T20:45:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 21, 2015 at 12:01:27PM -0500, Ramkumar Ramachandra wrote:\n\n> Instead of manually eyeballing style in reviews, just ask all\n> contributors to run their patches through [git-]clang-format.\n\nThanks for mentioning this; I hadn't seen the tool before.\n\nI didn't see it mentioned here, but for those who are also new to the\ntool, it has modes both for checking the content itself as well as diffs\n(so you are not stuck wading through its reformats of code you didn't\ntouch).\n\n> +BreakBeforeBraces: Linux\n> [...]\n> +BreakBeforeBraces: Stroustrup\n\nThese seem conflicting. It looks like you added \"Stroustrup\" to keep the\nbrace on the line with the \"struct\" keyword. But this does the wrong\nthing for \"cuddled else\"s like:\n\n  if (...) {\n     ...\n  } else {\n     ...\n  }\n\nI don't think clang-format has a mode that expresses our style.\n\nI ran some of my recent patches through clang-format-diff, and it\ngenerated quite a bit of output. Here are a few notes on what I saw.\nFeel free to ignore. They are not your problem, but others evaluating\nthe tool might find it useful (and a few of them might suggest some\nsettings for .clang-format).\n\n - It really wants to break function declarations that go over the\n   column limit, even though we often do not do so. I think we're pretty\n   inconsistent here, and I'd be fine going either way with it.\n\n - It really wanted to left-align some of my asterisks, like:\n\n     struct foo_list {\n       ...\n     } * foo, **foo_tail;\n\n   The odd thing is that it gets the second one right, but not the first\n   one (which should be \"*foo\" with no space). Setting:\n\n     DerivePointerAlignment: false\n     PointerAlignment: Right\n\n   cleared it up, but I'm curious why the auto-deriver didn't work.\n\n - It really doesn't like list-alignment, like:\n\n      #define FOO    1\n      #define LONGER 2\n\n   and would prefer only a single space between \"FOO\" and \"1\". I think\n   I'm OK with that, but we have a lot of aligned bits in the existing\n   code.\n\n - It really wants to put function __attribute__ macros on the same line\n   as the function. We often have it on a line above (especially it can\n   be so long). I couldn't find a way to specify this.\n\n - I had a long ternary operator broken across three lines, like:\n\n     foo = bar ?\n           some_long_thing(...) :\n\t   some_other_long_thing(...);\n\n   It put it all on one long line, which was much less readable. I set\n   BreakBeforeTernaryOperators to \"true\", but it did nothing. I set it\n   to \"false\", and then it broke. Which seems like a bug. It also\n   insisted on indenting it like:\n\n     foo = bar ?\n                   some_long_thing(...) :\n\t\t   some_other_long_thing(...);\n\n    which I found less readable.\n\nSo overall I think it has some promise, but I do not think it is quite\nflexible enough yet for us to use day-to-day. I'm slightly dubious that\nany automated formatter can ever be _perfect_ (sometimes\nhuman-subjective readability trumps a hard-and-fast rule), but this\nseems like it might have some promise. And over other indenters I have\nseen:\n\n  1. It's built on clang, so we know the parsing is solid.\n\n  2. It can operate on patches (and generates patches for you to apply!\n     You could add a git-add--interactive mode to selectively take its\n     suggestions).\n\nAgain, thanks for sharing.\n\n-Peff\n"},{"id":"255033","messageId":"CALkWK0knxJ5VTJoKhR_t4GS7pfg6PPYox9Srf3bvaX=m+sjqVw@mail.gmail.com","threadId":"38394","inReplyTo":"20150121204502.GA3287@peff.net","subject":"Re: [PATCH v2] .clang-format: introduce the use of clang-format","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2015-01-21T21:28:00Z","receivedAt":"2015-01-21T21:28:00Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Jeff King wrote:\n> On Wed, Jan 21, 2015 at 12:01:27PM -0500, Ramkumar Ramachandra wrote:\n>> +BreakBeforeBraces: Linux\n>> [...]\n>> +BreakBeforeBraces: Stroustrup\n\nOh, oops.\n\n>  - It really wants to break function declarations that go over the\n>    column limit, even though we often do not do so. I think we're pretty\n>    inconsistent here, and I'd be fine going either way with it.\n>\n>  - It really wanted to left-align some of my asterisks, like:\n>\n>      struct foo_list {\n>        ...\n>      } * foo, **foo_tail;\n>\n>    The odd thing is that it gets the second one right, but not the first\n>    one (which should be \"*foo\" with no space). Setting:\n>\n>      DerivePointerAlignment: false\n>      PointerAlignment: Right\n>\n>    cleared it up, but I'm curious why the auto-deriver didn't work.\n\nSounds like a bug.\n\n>  - It really doesn't like list-alignment, like:\n>\n>       #define FOO    1\n>       #define LONGER 2\n>\n>    and would prefer only a single space between \"FOO\" and \"1\". I think\n>    I'm OK with that, but we have a lot of aligned bits in the existing\n>    code.\n>\n>  - It really wants to put function __attribute__ macros on the same line\n>    as the function. We often have it on a line above (especially it can\n>    be so long). I couldn't find a way to specify this.\n\nYou have to compromise a bit if you want to use an auto-formatting\ntool, without losing your head patching every little detail :)\n\n>  - I had a long ternary operator broken across three lines, like:\n>\n>      foo = bar ?\n>            some_long_thing(...) :\n>            some_other_long_thing(...);\n>\n>    It put it all on one long line, which was much less readable. I set\n>    BreakBeforeTernaryOperators to \"true\", but it did nothing. I set it\n>    to \"false\", and then it broke. Which seems like a bug. It also\n>    insisted on indenting it like:\n>\n>      foo = bar ?\n>                    some_long_thing(...) :\n>                    some_other_long_thing(...);\n>\n>     which I found less readable.\n\nTo be honest, the LLVM community doesn't fix bugs just because they\ncan be fixed: it's quite heavily driven by commercial interest. And I\nreally don't find long ternary operators in a modern C++ codebase.\n\n> I'm slightly dubious that\n> any automated formatter can ever be _perfect_ (sometimes\n> human-subjective readability trumps a hard-and-fast rule), but this\n> seems like it might have some promise.\n\nIt works almost perfectly for the LLVM umbrella of projects. When they\nwant to change a coding convention (like leading Uppercase for\nvariable names), they write a clang-tidy thing to do it automatically.\n\n> So overall I think it has some promise, but I do not think it is quite\n> flexible enough yet for us to use day-to-day.\n\nThe big negative is that it will probably never be. I'll try to look\nat the larger issues later this week, if you can compromise on the\nfine details that are probably too hard to fix.\n\n>   2. It can operate on patches (and generates patches for you to apply!\n>      You could add a git-add--interactive mode to selectively take its\n>      suggestions).\n\nThere's a git-clang-format in the $CLANG_ROOT/tools/clang-format/. I do:\n\n   $ g cf @~\n\n... with the appropriate aliases.\n"},{"id":"255035","messageId":"20150121220903.GA10267@peff.net","threadId":"38394","inReplyTo":"CALkWK0knxJ5VTJoKhR_t4GS7pfg6PPYox9Srf3bvaX=m+sjqVw@mail.gmail.com","subject":"Re: [PATCH v2] .clang-format: introduce the use of clang-format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-21T22:09:03Z","receivedAt":"2015-01-21T22:09:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 21, 2015 at 04:28:00PM -0500, Ramkumar Ramachandra wrote:\n\n> > So overall I think it has some promise, but I do not think it is quite\n> > flexible enough yet for us to use day-to-day.\n> \n> The big negative is that it will probably never be. I'll try to look\n> at the larger issues later this week, if you can compromise on the\n> fine details that are probably too hard to fix.\n\nThe key to me is that we do not have necessarily take every suggestion\nthe tool makes. So it does not have to be perfect, just \"pretty good\".\n\nBut...I think it is not quite so simple. The clang-format-diff script\n(and git-clang-format) _do_ seem to operate on more than just the lines\nI've changed. I'm not clear on whether they're examining the whole file\n(just with the patches applied), or there's something in between\nhappening.\n\nSo rejecting the tool's suggestion one day may mean it suggests the same\nchange to you any other time you touch nearby parts of the file, which\ncould be annoying.\n\n> >   2. It can operate on patches (and generates patches for you to apply!\n> >      You could add a git-add--interactive mode to selectively take its\n> >      suggestions).\n> \n> There's a git-clang-format in the $CLANG_ROOT/tools/clang-format/. I do:\n> \n>    $ g cf @~\n> \n> ... with the appropriate aliases.\n\nNeat. Debian's package does not ship with that. I hacked-in very\nrudimentary interactive-add support for clang-format-diff (below) before\ngetting your response. It would be better built around \"git-clang-format\n--diff\" (though that script would need to be taught to do the right\nthing with the --color argument).\n\nHowever, because of the \"suggest the same change\" thing I mentioned\nabove, I am not sure whether interactively selecting is a good idea or\nnot.  You might end up having to say \"no\" to the same suggestions a lot.\n\nAnyway, here it is, for reference. You can use it like:\n\n  git add--interactive --patch=format --\n\nand you could probably even stick an \"exec\" line into an interactive\nrebase to go through and fixup individual patches in a whole series.\n\n---\ndiff --git a/.gitignore b/.gitignore\nindex a052419..6f5b815 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -30,6 +30,7 @@\n /git-checkout-index\n /git-cherry\n /git-cherry-pick\n+/git-clang-format-diff\n /git-clean\n /git-clone\n /git-column\ndiff --git a/Makefile b/Makefile\nindex c44eb3a..113534e 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -455,6 +455,7 @@ unexport CDPATH\n \n SCRIPT_SH += git-am.sh\n SCRIPT_SH += git-bisect.sh\n+SCRIPT_SH += git-clang-format-diff.sh\n SCRIPT_SH += git-difftool--helper.sh\n SCRIPT_SH += git-filter-branch.sh\n SCRIPT_SH += git-merge-octopus.sh\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex c725674..fd83adf 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -167,6 +167,16 @@ my %patch_modes = (\n \t\tFILTER => undef,\n \t\tIS_REVERSE => 0,\n \t},\n+\t'format' => {\n+\t\tDIFF => 'clang-format-diff',\n+\t\tAPPLY => sub { apply_patch_for_checkout_commit '', @_ },\n+\t\tAPPLY_CHECK => 'apply',\n+\t\tVERB => 'Apply',\n+\t\tTARGET => 'to index and worktree',\n+\t\tPARTICIPLE => 'applying',\n+\t\tFILTER => undef,\n+\t\tIS_REVERSE => 0\n+\t},\n );\n \n my %patch_mode_flavour = %{$patch_modes{stage}};\n@@ -1591,6 +1601,15 @@ sub process_args {\n \t\t\t\t\t\t       'checkout_head' : 'checkout_nothead');\n \t\t\t\t\t$arg = shift @ARGV or die \"missing --\";\n \t\t\t\t}\n+\t\t\t} elsif ($1 eq 'format') {\n+\t\t\t\t$patch_mode = $1;\n+\t\t\t\t$arg = shift @ARGV or die \"missing --\";\n+\t\t\t\tif ($arg eq '--') {\n+\t\t\t\t\t$patch_mode_revision = 'HEAD^';\n+\t\t\t\t} else {\n+\t\t\t\t\t$patch_mode_revision = $arg;\n+\t\t\t\t\t$arg = shift @ARGV or die \"missing --\";\n+\t\t\t\t}\n \t\t\t} elsif ($1 eq 'stage' or $1 eq 'stash') {\n \t\t\t\t$patch_mode = $1;\n \t\t\t\t$arg = shift @ARGV or die \"missing --\";\ndiff --git a/git-clang-format-diff.sh b/git-clang-format-diff.sh\nnew file mode 100755\nindex 0000000..9351883\n--- /dev/null\n+++ b/git-clang-format-diff.sh\n@@ -0,0 +1,20 @@\n+#!/bin/sh\n+\n+# This is what it's called in the Debian package, but it seems\n+# like there ought to be a symlink without the version...\n+CFD=clang-format-diff-3.6\n+\n+# Strip out --color, as clang's patch reader cannot handle it.\n+# Robustly handling arrays in bourne shell is insane.\n+eval \"set -- $(\n+\tfor i in \"$@\"; do\n+\t\ttest \"--color\" = \"$i\" && continue\n+\t\tprintf \" '\"\n+\t\tprintf '%s' \"$i\" | sed \"s/'/'\\\\\\\\''/g\"\n+\t\tprintf \"'\"\n+\tdone\n+)\"\n+\n+git diff-index -p \"$@\" |\n+$CFD -p1 |\n+sed -e 's,^--- ,&a/,' -e 's,^+++ ,&b/,'\n"}]}