Re: [PATCH v2] .clang-format: introduce the use of clang-format
On Wed, Jan 21, 2015 at 12:01:27PM -0500, Ramkumar Ramachandra wrote:
Instead of manually eyeballing style in reviews, just ask all
contributors to run their patches through [git-]clang-format.
Thanks for mentioning this; I hadn't seen the tool before.
I didn't see it mentioned here, but for those who are also new to the
tool, it has modes both for checking the content itself as well as diffs
(so you are not stuck wading through its reformats of code you didn't
touch).
+BreakBeforeBraces: Linux
[...]
+BreakBeforeBraces: Stroustrup
These seem conflicting. It looks like you added Stroustrup to keep the
brace on the line with the struct keyword. But this does the wrong
thing for cuddled elses like:
if (...) {
...
} else {
...
}
I don't think clang-format has a mode that expresses our style.
I ran some of my recent patches through clang-format-diff, and it
generated quite a bit of output. Here are a few notes on what I saw.
Feel free to ignore. They are not your problem, but others evaluating
the tool might find it useful (and a few of them might suggest some
settings for .clang-format).
- It really wants to break function declarations that go over the
column limit, even though we often do not do so. I think we're pretty
inconsistent here, and I'd be fine going either way with it.
- It really wanted to left-align some of my asterisks, like:
struct foo_list {
...
} * foo, **foo_tail;
The odd thing is that it gets the second one right, but not the first
one (which should be *foo with no space). Setting:
DerivePointerAlignment: false
PointerAlignment: Right
cleared it up, but I'm curious why the auto-deriver didn't work.
- It really doesn't like list-alignment, like:
#define FOO1
#define LONGER 2
and would prefer only a single space between FOO and 1. I think
I'm OK with that, but we have a lot of aligned bits in the existing
code.
- It really wants to put function __attribute__ macros on the same line
as the function. We often have it on a line above (especially it can
be so long). I couldn't find a way to specify this.
- I had a long ternary operator broken across three lines, like:
foo = bar ?
some_long_thing(...) :
some_other_long_thing(...);
It put it all on one long line, which was much less readable. I set
BreakBeforeTernaryOperators to true, but it did nothing. I set it
to false, and then it broke. Which seems like a bug. It also
insisted on indenting it like:
foo = bar ?
some_long_thing(...) :
some_other_long_thing(...);
which I found less readable.
So overall I think it has some promise, but I do not think it is quite
flexible enough yet for us to use day-to-day. I'm slightly dubious that
any automated formatter can ever be _perfect_ (sometimes
human-subjective readability trumps a hard-and-fast rule), but this
seems like it might have some promise. And over other indenters I have
seen:
1. It's built on clang, so we know the parsing is solid.
2. It can operate on patches (and generates patches for you to apply!
You could add a git-add--interactive mode to selectively take its
suggestions).
Again, thanks for sharing.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] .clang-format: introduce the use of clang-format
Jeff King wrote:
On Wed, Jan 21, 2015 at 12:01:27PM -0500, Ramkumar Ramachandra wrote:
+BreakBeforeBraces: Linux
[...]
+BreakBeforeBraces: Stroustrup
Oh, oops.
- It really wants to break function declarations that go over the
column limit, even though we often do not do so. I think we're pretty
inconsistent here, and I'd be fine going either way with it.
- It really wanted to left-align some of my asterisks, like:
struct foo_list {
...
} * foo, **foo_tail;
The odd thing is that it gets the second one right, but not the first
one (which should be *foo with no space). Setting:
DerivePointerAlignment: false
PointerAlignment: Right
cleared it up, but I'm curious why the auto-deriver didn't work.
Sounds like a bug.
- It really doesn't like list-alignment, like:
#define FOO1
#define LONGER 2
and would prefer only a single space between FOO and 1. I think
I'm OK with that, but we have a lot of aligned bits in the existing
code.
- It really wants to put function __attribute__ macros on the same line
as the function. We often have it on a line above (especially it can
be so long). I couldn't find a way to specify this.
You have to compromise a bit if you want to use an auto-formatting
tool, without losing your head patching every little detail :)
- I had a long ternary operator broken across three lines, like:
foo = bar ?
some_long_thing(...) :
some_other_long_thing(...);
It put it all on one long line, which was much less readable. I set
BreakBeforeTernaryOperators to true, but it did nothing. I set it
to false, and then it broke. Which seems like a bug. It also
insisted on indenting it like:
foo = bar ?
some_long_thing(...) :
some_other_long_thing(...);
which I found less readable.
To be honest, the LLVM community doesn't fix bugs just because they
can be fixed: it's quite heavily driven by commercial interest. And I
really don't find long ternary operators in a modern C++ codebase.
I'm slightly dubious that
any automated formatter can ever be _perfect_ (sometimes
human-subjective readability trumps a hard-and-fast rule), but this
seems like it might have some promise.
It works almost perfectly for the LLVM umbrella of projects. When they
want to change a coding convention (like leading Uppercase for
variable names), they write a clang-tidy thing to do it automatically.
So overall I think it has some promise, but I do not think it is quite
flexible enough yet for us to use day-to-day.
The big negative is that it will probably never be. I'll try to look
at the larger issues later this week, if you can compromise on the
fine details that are probably too hard to fix.
2. It can operate on patches (and generates patches for you to apply!
You could add a git-add--interactive mode to selectively take its
suggestions).
There's a git-clang-format in the $CLANG_ROOT/tools/clang-format/. I do:
$ g cf @~
... with the appropriate aliases.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] .clang-format: introduce the use of clang-format
On Wed, Jan 21, 2015 at 04:28:00PM -0500, Ramkumar Ramachandra wrote:
So overall I think it has some promise, but I do not think it is quite
flexible enough yet for us to use day-to-day.
The big negative is that it will probably never be. I'll try to look
at the larger issues later this week, if you can compromise on the
fine details that are probably too hard to fix.
The key to me is that we do not have necessarily take every suggestion
the tool makes. So it does not have to be perfect, just pretty good.
But...I think it is not quite so simple. The clang-format-diff script
(and git-clang-format) _do_ seem to operate on more than just the lines
I've changed. I'm not clear on whether they're examining the whole file
(just with the patches applied), or there's something in between
happening.
So rejecting the tool's suggestion one day may mean it suggests the same
change to you any other time you touch nearby parts of the file, which
could be annoying.
2. It can operate on patches (and generates patches for you to apply!
You could add a git-add--interactive mode to selectively take its
suggestions).
There's a git-clang-format in the $CLANG_ROOT/tools/clang-format/. I do:
$ g cf @~
... with the appropriate aliases.
Neat. Debian's package does not ship with that. I hacked-in very
rudimentary interactive-add support for clang-format-diff (below) before
getting your response. It would be better built around git-clang-format
--diff (though that script would need to be taught to do the right
thing with the --color argument).
However, because of the suggest the same change thing I mentioned
above, I am not sure whether interactively selecting is a good idea or
not. You might end up having to say no to the same suggestions a lot.
Anyway, here it is, for reference. You can use it like:
git add--interactive --patch=format --
and you could probably even stick an exec line into an interactive
rebase to go through and fixup individual patches in a whole series.
---
diff --git a/.gitignore b/.gitignore
index a052419..6f5b815 100644
--- a/.gitignore
+++ b/.gitignore
@@ -30,6 +30,7 @@
/git-checkout-index
/git-cherry
/git-cherry-pick
+/git-clang-format-diff
/git-clean
/git-clone
/git-column
diff --git a/Makefile b/Makefile
index c44eb3a..113534e 100644
--- a/Makefile
+++ b/Makefile
@@ -455,6 +455,7 @@ unexport CDPATH
SCRIPT_SH += git-am.sh
SCRIPT_SH += git-bisect.sh
+SCRIPT_SH += git-clang-format-diff.sh
SCRIPT_SH += git-difftool--helper.sh
SCRIPT_SH += git-filter-branch.sh
SCRIPT_SH += git-merge-octopus.sh
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index c725674..fd83adf 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -167,6 +167,16 @@ my %patch_modes = (
FILTER = undef,
IS_REVERSE = 0,
},
+ 'format' = {
+ DIFF = 'clang-format-diff',
+ APPLY = sub { apply_patch_for_checkout_commit '', @_ },
+ APPLY_CHECK = 'apply',
+ VERB = 'Apply',
+ TARGET = 'to index and worktree',
+ PARTICIPLE = 'applying',
+ FILTER = undef,
+ IS_REVERSE = 0
+ },
);
my %patch_mode_flavour = %{$patch_modes{stage}};
@@ -1591,6 +1601,15 @@ sub process_args {
'checkout_head' :
'checkout_nothead');
$arg = shift @ARGV or die missing --;
}
+ } elsif ($1 eq 'format') {
+ $patch_mode = $1;
+ $arg = shift @ARGV or die missing --;
+ if ($arg eq '--') {
+ $patch_mode_revision = 'HEAD^';
+ } else {
+ $patch_mode_revision = $arg;
+ $arg = shift @ARGV or die missing --;
+ }
} elsif ($1 eq 'stage' or $1 eq 'stash') {
$patch_mode = $1;
$arg = shift @ARGV or die missing --;
diff --git a/git-clang-format-diff.sh b/git-clang-format-diff.sh
new file mode 100755
index 000..9351883
--- /dev/null
+++ b/git-clang-format-diff.sh
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+# This is what it's called in the Debian package, but it seems
+# like there ought to be a symlink without the version...
+CFD=clang-format-diff-3.6
+
+# Strip out --color, as clang's patch reader cannot handle it.
+# Robustly handling arrays in bourne shell is insane.
+eval set -- $(
+ for i in $@; do
+ test --color = $i continue
+ printf '
+ printf '%s' $i | sed s/'/'''/g
+ printf '
+ done
+)
+
+git diff-index -p $@ |
+$CFD -p1 |
+sed -e 's,^--- ,a/,' -e 's,^+++ ,b/,'
--
To
