[v4,01/12] Add .clang-format with defaults matching what's specified in the style guide

Submitted by Christophe de Dinechin on Feb. 15, 2018, 4:06 p.m.

Details

Message ID 20180215160625.75406-2-christophe@dinechin.org
State New
Headers show
Series "Updates to the style guide" ( rev: 3 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 15, 2018, 4:06 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

There are two use cases for this .clang-format:

1. Local reformatting of source code, e.g. using Emacs clang-format.el package.
   This is basically a wawy to accelerate routine reformatting actions during
   heavy editing or refactoring.

2. Global reformatting of the source code, e.g. using the 'make reformat'
   rule in the 'build' makefiles.

Presently, our source code is not entirely ready for this kind of
refactoring yet, mostly because several of our headers are not
self-contained, so you end up with errors like:

    In file included from agent-msg-filter.c:25:
    In file included from ./red-common.h:37:
    In file included from ./spice.h:27:
    ./spice-migration.h:47:31: error: unknown type name 'SpiceServer'

Changes since v3:
- Documented rationale and use cases in commit log
- Some setting changes to more closely match existing server practice

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 .clang-format | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 .clang-format

Patch hide | download patch | download mbox

diff --git a/.clang-format b/.clang-format
new file mode 100644
index 00000000..56835831
--- /dev/null
+++ b/.clang-format
@@ -0,0 +1,30 @@ 
+Language:        Cpp
+# BasedOnStyle:  LLVM
+
+# The following is commented out until widely supported
+# IncludeBlocks: Regroup
+SortIncludes: true
+
+IncludeCategories:
+  - Regex:           'config.h'
+    Priority:        -1
+  - Regex:           '^"spice.*"'
+    Priority:        1
+  - Regex:           'glib'
+    Priority:        4
+  - Regex:           '^<.*>'
+    Priority:        3
+  - Regex:           '^".*"'
+    Priority:        2
+
+ColumnLimit:     100
+IndentCaseLabels: false
+IndentWidth:     4
+BreakBeforeBraces: Mozilla
+
+BinPackParameters: false
+
+# For global reformatting, should we consider 'true' for these?
+# It helps in many parts of the code but breaks other
+AlignConsecutiveAssignments: false
+AlignConsecutiveDeclarations: false

Comments

On Thu, Feb 15, 2018 at 05:06:14PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> There are two use cases for this .clang-format:
> 
> 1. Local reformatting of source code, e.g. using Emacs clang-format.el package.
>    This is basically a wawy to accelerate routine reformatting actions during

'way'

>    heavy editing or refactoring.
> 
> 2. Global reformatting of the source code, e.g. using the 'make reformat'
>    rule in the 'build' makefiles.
> 
> Presently, our source code is not entirely ready for this kind of
> refactoring yet, mostly because several of our headers are not
> self-contained, so you end up with errors like:
> 
>     In file included from agent-msg-filter.c:25:
>     In file included from ./red-common.h:37:
>     In file included from ./spice.h:27:
>     ./spice-migration.h:47:31: error: unknown type name 'SpiceServer'

My understanding was also that this .clang-format file got the
formatting wrong in some cases when applied over the codebase?



> Changes since v3:
> - Documented rationale and use cases in commit log
> - Some setting changes to more closely match existing server practice

NB: This part belongs below the -- line as this usually should not be pushed
to the upstream repository
> On 16 Feb 2018, at 10:07, Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> On Thu, Feb 15, 2018 at 05:06:14PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> There are two use cases for this .clang-format:
>> 
>> 1. Local reformatting of source code, e.g. using Emacs clang-format.el package.
>>   This is basically a wawy to accelerate routine reformatting actions during
> 
> 'way'
> 
>>   heavy editing or refactoring.
>> 
>> 2. Global reformatting of the source code, e.g. using the 'make reformat'
>>   rule in the 'build' makefiles.
>> 
>> Presently, our source code is not entirely ready for this kind of
>> refactoring yet, mostly because several of our headers are not
>> self-contained, so you end up with errors like:
>> 
>>    In file included from agent-msg-filter.c:25:
>>    In file included from ./red-common.h:37:
>>    In file included from ./spice.h:27:
>>    ./spice-migration.h:47:31: error: unknown type name 'SpiceServer'
> 
> My understanding was also that this .clang-format file got the
> formatting wrong in some cases when applied over the codebase?

I did improve the settings, see below “some setting changes”.

Frediano’s comment was based on a push where I had not yet noticed that

1. LLVM defaults to aligning decls, so I added AlignConsecutiveAssignments: false and AlignConsecutiveDeclarations: false.
2. The ‘Linux’ brace alignment was not correct for structs, which prompted me to change it that setting to ‘Mozilla'

The result after these changes is here: https://gitlab.com/c3d/spice-server/commit/19e7c5bb97b0ce9d489c16229ab9caf39080145d

There are still issues, but they seem to be more related to:

- Misinterpretation of some macro code, e.g. https://gitlab.com/c3d/spice-server/commit/19e7c5bb97b0ce9d489c16229ab9caf39080145d#c75ac8e2c001e819bae810b126f19ba02f1bac2a_35_35

- Special manual formatting, e.g. https://gitlab.com/c3d/spice-server/commit/19e7c5bb97b0ce9d489c16229ab9caf39080145d#1f4f5fab2bf5fd2a2296b15eba820fc32802b59d_239_235. Those can be protected with a "// clang-format off” comment.

- The return type on a separate line is systematically removed. I don’t know if we want that or not. We could set AlwaysBreakAfterDefinitionReturnType: TopLevel.

For the rest, I think it’s doing a rather good job, and there are many places where it markedly improves things.


> 
> 
> 
>> Changes since v3:
>> - Documented rationale and use cases in commit log
>> - Some setting changes to more closely match existing server practice
> 
> NB: This part belongs below the -- line as this usually should not be pushed
> to the upstream repository

Ah, yes. Thanks.

> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel