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

Submitted by Christophe de Dinechin on Feb. 8, 2018, 11:25 a.m.

Details

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

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 8, 2018, 11:25 a.m.
From: Christophe de Dinechin <dinechin@redhat.com>

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 .clang-format | 23 +++++++++++++++++++++++
 1 file changed, 23 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..91203600
--- /dev/null
+++ b/.clang-format
@@ -0,0 +1,23 @@ 
+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: Linux

Comments

Shouldn't this go with a Makefile rule? A few lines in the log what this
is about, what is the goal for having this file, ... would not hurt.

Christophe

On Thu, Feb 08, 2018 at 12:25:21PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  .clang-format | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644 .clang-format
> 
> diff --git a/.clang-format b/.clang-format
> new file mode 100644
> index 00000000..91203600
> --- /dev/null
> +++ b/.clang-format
> @@ -0,0 +1,23 @@
> +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: Linux
> -- 
> 2.13.5 (Apple Git-94)
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> Shouldn't this go with a Makefile rule? A few lines in the log what this
> is about, what is the goal for having this file, ... would not hurt.
> 
> Christophe
> 

I think this file is supposed to just help developers so should not
be in the Makefile.
I think you mean that the intention should be written in the commit message.

> On Thu, Feb 08, 2018 at 12:25:21PM +0100, Christophe de Dinechin wrote:
> > From: Christophe de Dinechin <dinechin@redhat.com>
> > 
> > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > ---
> >  .clang-format | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >  create mode 100644 .clang-format
> > 
> > diff --git a/.clang-format b/.clang-format
> > new file mode 100644
> > index 00000000..91203600
> > --- /dev/null
> > +++ b/.clang-format
> > @@ -0,0 +1,23 @@
> > +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: Linux

Frediano
On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote:
> > 
> > Shouldn't this go with a Makefile rule? A few lines in the log what this
> > is about, what is the goal for having this file, ... would not hurt.
> > 
> > Christophe
> > 
> 
> I think this file is supposed to just help developers so should not
> be in the Makefile.

Yes, after reading various threads, it's apparently meant to be used
together with emacs for formatting of small code blocks, it's not usable
on the whole codebase. So a 'make clang-format' rule apparently would
not make sense.

> I think you mean that the intention should be written in the commit message.

Yes, knowing how it's meant to be used, why we want it in the codebase.

Christophe

> 
> > On Thu, Feb 08, 2018 at 12:25:21PM +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > 
> > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > > ---
> > >  .clang-format | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >  create mode 100644 .clang-format
> > > 
> > > diff --git a/.clang-format b/.clang-format
> > > new file mode 100644
> > > index 00000000..91203600
> > > --- /dev/null
> > > +++ b/.clang-format
> > > @@ -0,0 +1,23 @@
> > > +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: Linux
> 
> Frediano
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> On 14 Feb 2018, at 17:34, Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote:
>>> 
>>> Shouldn't this go with a Makefile rule? A few lines in the log what this
>>> is about, what is the goal for having this file, ... would not hurt.
>>> 
>>> Christophe
>>> 
>> 
>> I think this file is supposed to just help developers so should not
>> be in the Makefile.
> 
> Yes, after reading various threads, it's apparently meant to be used
> together with emacs for formatting of small code blocks, it's not usable
> on the whole codebase. So a 'make clang-format' rule apparently would
> not make sense.
> 
>> I think you mean that the intention should be written in the commit message.
> 
> Yes, knowing how it's meant to be used, why we want it in the codebase.

Why would we NOT want it in the codebase?

> 
> Christophe
> 
>> 
>>> On Thu, Feb 08, 2018 at 12:25:21PM +0100, Christophe de Dinechin wrote:
>>>> From: Christophe de Dinechin <dinechin@redhat.com>
>>>> 
>>>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>>>> ---
>>>> .clang-format | 23 +++++++++++++++++++++++
>>>> 1 file changed, 23 insertions(+)
>>>> create mode 100644 .clang-format
>>>> 
>>>> diff --git a/.clang-format b/.clang-format
>>>> new file mode 100644
>>>> index 00000000..91203600
>>>> --- /dev/null
>>>> +++ b/.clang-format
>>>> @@ -0,0 +1,23 @@
>>>> +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: Linux
>> 
>> Frediano
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 14 Feb 2018, at 17:34, Christophe Fergeau <cfergeau@redhat.com> wrote:
> > 
> > On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote:
> >>> 
> >>> Shouldn't this go with a Makefile rule? A few lines in the log what this
> >>> is about, what is the goal for having this file, ... would not hurt.
> >>> 
> >>> Christophe
> >>> 
> >> 
> >> I think this file is supposed to just help developers so should not
> >> be in the Makefile.
> > 
> > Yes, after reading various threads, it's apparently meant to be used
> > together with emacs for formatting of small code blocks, it's not usable
> > on the whole codebase. So a 'make clang-format' rule apparently would
> > not make sense.
> > 
> >> I think you mean that the intention should be written in the commit message.
> > 
> > Yes, knowing how it's meant to be used, why we want it in the codebase.
> 
> Why would we NOT want it in the codebase?

I'm not saying I'm against what this patch is adding. I'm saying
I'm against a commit adding something without much of a rationale in its
commit log.

Christophe
> On 15 Feb 2018, at 10:07, Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote:
>> 
>> 
>>> On 14 Feb 2018, at 17:34, Christophe Fergeau <cfergeau@redhat.com> wrote:
>>> 
>>> On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote:
>>>>> 
>>>>> Shouldn't this go with a Makefile rule? A few lines in the log what this
>>>>> is about, what is the goal for having this file, ... would not hurt.
>>>>> 
>>>>> Christophe
>>>>> 
>>>> 
>>>> I think this file is supposed to just help developers so should not
>>>> be in the Makefile.
>>> 
>>> Yes, after reading various threads, it's apparently meant to be used
>>> together with emacs for formatting of small code blocks, it's not usable
>>> on the whole codebase. So a 'make clang-format' rule apparently would
>>> not make sense.
>>> 
>>>> I think you mean that the intention should be written in the commit message.
>>> 
>>> Yes, knowing how it's meant to be used, why we want it in the codebase.
>> 
>> Why would we NOT want it in the codebase?
> 
> I'm not saying I'm against what this patch is adding. I'm saying
> I'm against a commit adding something without much of a rationale in its
> commit log.

Fair point. Added that.

To clarify, right now, I don’t think we can refactor whole files yet, the code is not ready for it. See https://gitlab.com/c3d/spice-server/commit/f6297e63f5e785e5fdf7176fd9381d0f465f6c11 for a review of the server changes, if you are curious.

Some of the issues include:

- Our headers are not yet self-contained, so you get:
	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'

- Some comments get munged, notably comments at end of line:

- Minor things, like a macro where (id) & CONSTANT interpreted as a cast (id) with address taken and reformatted accordingly

Christophe
> 
> > On 15 Feb 2018, at 10:07, Christophe Fergeau <cfergeau@redhat.com> wrote:
> > 
> > On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote:
> >> 
> >> 
> >>> On 14 Feb 2018, at 17:34, Christophe Fergeau <cfergeau@redhat.com> wrote:
> >>> 
> >>> On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote:
> >>>>> 
> >>>>> Shouldn't this go with a Makefile rule? A few lines in the log what
> >>>>> this
> >>>>> is about, what is the goal for having this file, ... would not hurt.
> >>>>> 
> >>>>> Christophe
> >>>>> 
> >>>> 
> >>>> I think this file is supposed to just help developers so should not
> >>>> be in the Makefile.
> >>> 
> >>> Yes, after reading various threads, it's apparently meant to be used
> >>> together with emacs for formatting of small code blocks, it's not usable
> >>> on the whole codebase. So a 'make clang-format' rule apparently would
> >>> not make sense.
> >>> 
> >>>> I think you mean that the intention should be written in the commit
> >>>> message.
> >>> 
> >>> Yes, knowing how it's meant to be used, why we want it in the codebase.
> >> 
> >> Why would we NOT want it in the codebase?
> > 
> > I'm not saying I'm against what this patch is adding. I'm saying
> > I'm against a commit adding something without much of a rationale in its
> > commit log.
> 
> Fair point. Added that.
> 
> To clarify, right now, I don’t think we can refactor whole files yet, the
> code is not ready for it. See
> https://gitlab.com/c3d/spice-server/commit/f6297e63f5e785e5fdf7176fd9381d0f465f6c11
> for a review of the server changes, if you are curious.
> 

I think the main reason is that you are attempting to change the
current style entirely.
For instance we have a section that explicitly state we don't
want column alignment.
Or see the section about section declaration.

> Some of the issues include:
> 
> - Our headers are not yet self-contained, so you get:
> 	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'
> 

I test quite often and as far as I know they are self contained.
In this case spice-migration.h should be included from spice.h
and SpiceServer is defined in spice-server.h included from spice.h
before spice-migration.h.

> - Some comments get munged, notably comments at end of line:
> 
> - Minor things, like a macro where (id) & CONSTANT interpreted as a cast (id)
> with address taken and reformatted accordingly
> 
> Christophe

If your intention with this file is changing the entire style
I would personally avoid it.

The subject is "Add .clang-format with defaults matching what's specified
in the style guide", from the branch you pointed out looks like is false.

Frediano
> On 15 Feb 2018, at 11:59, Frediano Ziglio <fziglio@redhat.com> wrote:
> 
>>> 
>>> On 15 Feb 2018, at 10:07, Christophe Fergeau <cfergeau@redhat.com> wrote:
>>> 
>>> On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote:
>>>> 
>>>> 
>>>>> On 14 Feb 2018, at 17:34, Christophe Fergeau <cfergeau@redhat.com> wrote:
>>>>> 
>>>>> On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote:
>>>>>>> 
>>>>>>> Shouldn't this go with a Makefile rule? A few lines in the log what
>>>>>>> this
>>>>>>> is about, what is the goal for having this file, ... would not hurt.
>>>>>>> 
>>>>>>> Christophe
>>>>>>> 
>>>>>> 
>>>>>> I think this file is supposed to just help developers so should not
>>>>>> be in the Makefile.
>>>>> 
>>>>> Yes, after reading various threads, it's apparently meant to be used
>>>>> together with emacs for formatting of small code blocks, it's not usable
>>>>> on the whole codebase. So a 'make clang-format' rule apparently would
>>>>> not make sense.
>>>>> 
>>>>>> I think you mean that the intention should be written in the commit
>>>>>> message.
>>>>> 
>>>>> Yes, knowing how it's meant to be used, why we want it in the codebase.
>>>> 
>>>> Why would we NOT want it in the codebase?
>>> 
>>> I'm not saying I'm against what this patch is adding. I'm saying
>>> I'm against a commit adding something without much of a rationale in its
>>> commit log.
>> 
>> Fair point. Added that.
>> 
>> To clarify, right now, I don’t think we can refactor whole files yet, the
>> code is not ready for it. See
>> https://gitlab.com/c3d/spice-server/commit/f6297e63f5e785e5fdf7176fd9381d0f465f6c11
>> for a review of the server changes, if you are curious.
>> 
> 
> I think the main reason is that you are attempting to change the
> current style entirely.

No, really not.

> For instance we have a section that explicitly state we don't
> want column alignment.

Yes, ooops. That was not in the branch when I originally pushed it for review, and then I experimented with that setting and did a push -f.

> Or see the section about section declaration.

?

> 
>> Some of the issues include:
>> 
>> - Our headers are not yet self-contained, so you get:
>> 	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'
>> 
> 
> I test quite often and as far as I know they are self contained.

I just proved they are not :-)

> In this case spice-migration.h should be included from spice.h
> and SpiceServer is defined in spice-server.h included from spice.h
> before spice-migration.h.

That is the very definition of “not self contained"

> 
>> - Some comments get munged, notably comments at end of line:
>> 
>> - Minor things, like a macro where (id) & CONSTANT interpreted as a cast (id)
>> with address taken and reformatted accordingly
>> 
>> Christophe
> 
> If your intention with this file is changing the entire style
> I would personally avoid it.

No. I did this experiment to get a feel of how close I was to the original style (assuming there really is one, which is not obvious from reading the code).

> 
> The subject is "Add .clang-format with defaults matching what's specified
> in the style guide", from the branch you pointed out looks like is false.

Not, it looks like it is not yet true ;-)

For instance, I noticed by doing this experiment that “Linux” is wrong, we want “Mozilla”. Also, I think we would need BinPackParameters: false. I experimented with the alignment to try to fix issues I was observing with the alignment of parameters, but as you point out, it also aligns decls. I personally like that, but that’s not what the style guide says.



> 
> Frediano
> 
> > On 15 Feb 2018, at 11:59, Frediano Ziglio <fziglio@redhat.com> wrote:
> > 
> >>> 
> >>> On 15 Feb 2018, at 10:07, Christophe Fergeau <cfergeau@redhat.com> wrote:
> >>> 
> >>> On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote:
> >>>> 
> >>>> 
> >>>>> On 14 Feb 2018, at 17:34, Christophe Fergeau <cfergeau@redhat.com>
> >>>>> wrote:
> >>>>> 
> >>>>> On Wed, Feb 14, 2018 at 10:45:56AM -0500, Frediano Ziglio wrote:
> >>>>>>> 
> >>>>>>> Shouldn't this go with a Makefile rule? A few lines in the log what
> >>>>>>> this
> >>>>>>> is about, what is the goal for having this file, ... would not hurt.
> >>>>>>> 
> >>>>>>> Christophe
> >>>>>>> 
> >>>>>> 
> >>>>>> I think this file is supposed to just help developers so should not
> >>>>>> be in the Makefile.
> >>>>> 
> >>>>> Yes, after reading various threads, it's apparently meant to be used
> >>>>> together with emacs for formatting of small code blocks, it's not
> >>>>> usable
> >>>>> on the whole codebase. So a 'make clang-format' rule apparently would
> >>>>> not make sense.
> >>>>> 
> >>>>>> I think you mean that the intention should be written in the commit
> >>>>>> message.
> >>>>> 
> >>>>> Yes, knowing how it's meant to be used, why we want it in the codebase.
> >>>> 
> >>>> Why would we NOT want it in the codebase?
> >>> 
> >>> I'm not saying I'm against what this patch is adding. I'm saying
> >>> I'm against a commit adding something without much of a rationale in its
> >>> commit log.
> >> 
> >> Fair point. Added that.
> >> 
> >> To clarify, right now, I don’t think we can refactor whole files yet, the
> >> code is not ready for it. See
> >> https://gitlab.com/c3d/spice-server/commit/f6297e63f5e785e5fdf7176fd9381d0f465f6c11
> >> for a review of the server changes, if you are curious.
> >> 
> > 
> > I think the main reason is that you are attempting to change the
> > current style entirely.
> 
> No, really not.
> 
> > For instance we have a section that explicitly state we don't
> > want column alignment.
> 
> Yes, ooops. That was not in the branch when I originally pushed it for
> review, and then I experimented with that setting and did a push -f.
> 
> > Or see the section about section declaration.
> 
> ?
> 

Sorry, the function declaration/definitions.

> > 
> >> Some of the issues include:
> >> 
> >> - Our headers are not yet self-contained, so you get:
> >> 	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'
> >> 
> > 
> > I test quite often and as far as I know they are self contained.
> 
> I just proved they are not :-)
> 

They are designed like glib headers, you have to include spice.h,
the only exception is the server code which has however to include
them in a proper order.

> > In this case spice-migration.h should be included from spice.h
> > and SpiceServer is defined in spice-server.h included from spice.h
> > before spice-migration.h.
> 
> That is the very definition of “not self contained"
> 

Yes, is the exception that prove the rule.

> > 
> >> - Some comments get munged, notably comments at end of line:
> >> 
> >> - Minor things, like a macro where (id) & CONSTANT interpreted as a cast
> >> (id)
> >> with address taken and reformatted accordingly
> >> 
> >> Christophe
> > 
> > If your intention with this file is changing the entire style
> > I would personally avoid it.
> 
> No. I did this experiment to get a feel of how close I was to the original
> style (assuming there really is one, which is not obvious from reading the
> code).
> 

Ok, then you post the wrong link to prove your point.

> > 
> > The subject is "Add .clang-format with defaults matching what's specified
> > in the style guide", from the branch you pointed out looks like is false.
> 
> Not, it looks like it is not yet true ;-)
> 
> For instance, I noticed by doing this experiment that “Linux” is wrong, we
> want “Mozilla”. Also, I think we would need BinPackParameters: false. I
> experimented with the alignment to try to fix issues I was observing with
> the alignment of parameters, but as you point out, it also aligns decls. I
> personally like that, but that’s not what the style guide says.
> 

There are a bit of inconsistency, indeed.
In some file some more "gobject/glib" style was followed (like column
indentation or parameter indentation).
That's why defining a "we'd like this style" but not be too restrictive
and more respectful of current code avoids to change everything.

Frediano