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 else"s 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 FOO 1
#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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html