patch.clang-format: introduce the use of clang-format

Instead of manually eyeballing style in reviews, just ask all contributors to run their patches through [git-]clang-format.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 The idea is to introduce the community to this new toy I found called
 clang-format. Whether or not it's actually going to be used doesn't
 bother me too much.

 I'm not 100% sure of the style, but I'll leave you to tweak that
 using http://clang.llvm.org/docs/ClangFormatStyleOptions.html

 The current code isn't terribly conformant, but I suppose that'll
 change with time.

 .clang-format | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 .clang-format

Am 17.01.2015 um 22:30 schrieb Ramkumar Ramachandra:

Why no newline on the last line?

These one would be needed as well to match our style, I think:

	AllowShortFunctionsOnASingleLine: None
	ContinuationIndentWidth: 8

And probably this one:

	Cpp11BracedListStyle: false

However, even then struct declarations that are combined with variable declaration and initialization get mangled:

	struct a {
		int n;
		const char *s;
	} arr[] = {
		{ 1, "one" },
		{ 2, "two" }
	};

becomes:

	struct a
	{
		int n;
		const char *s;
	} arr[] = { { 1, "one" }, { 2, "two" } };

It gets formatted better if arr is declared separately.

And this one helps get rid of the added line break between struct a and the following brace:

	BreakBeforeBraces: Stroustrup

Instead of manually eyeballing style in reviews, just ask all contributors to run their patches through [git-]clang-format.

However, struct declarations that are combined with variable declaration and initialization get mangled:

        struct a {
                int n;
                const char *s;
        } arr[] = {
                { 1, "one" },
                { 2, "two" }
        };

becomes:

        struct a {
                int n;
                const char *s;
        } arr[] = { { 1, "one" }, { 2, "two" } };

It gets formatted better if arr is declared separately.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 .clang-format | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 .clang-format

René Scharfe wrote:

I'm pretty sure this is a bug in clang-format. It might be semi-trivial to fix too.

Thanks for your inputs.

On Wed, Jan 21, 2015 at 12:01:27PM -0500, Ramkumar Ramachandra wrote:

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

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

Jeff King wrote:

Oh, oops.

Sounds like a bug.

You have to compromise a bit if you want to use an auto-formatting tool, without losing your head patching every little detail :)

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.

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.

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.

There's a git-clang-format in the $CLANG_ROOT/tools/clang-format/. I do:

   $ g cf @~

... with the appropriate aliases.

On Wed, Jan 21, 2015 at 04:28:00PM -0500, Ramkumar Ramachandra wrote:

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.

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.

---