Re: [Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide
> > > On 15 Feb 2018, at 11:59, Frediano Ziglio wrote: > > > >>> > >>> On 15 Feb 2018, at 10:07, Christophe Fergeau wrote: > >>> > >>> On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote: > > > > On 14 Feb 2018, at 17:34, Christophe Fergeau > > 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 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide
> On 15 Feb 2018, at 11:59, Frediano Ziglio wrote: > >>> >>> On 15 Feb 2018, at 10:07, Christophe Fergeau wrote: >>> >>> On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote: > On 14 Feb 2018, at 17:34, Christophe Fergeau 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 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide
> > > On 15 Feb 2018, at 10:07, Christophe Fergeau wrote: > > > > On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote: > >> > >> > >>> On 14 Feb 2018, at 17:34, Christophe Fergeau 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 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide
> On 15 Feb 2018, at 10:07, Christophe Fergeau wrote: > > On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote: >> >> >>> On 14 Feb 2018, at 17:34, Christophe Fergeau 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 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide
On Wed, Feb 14, 2018 at 10:29:25PM +0100, Christophe de Dinechin wrote: > > > > On 14 Feb 2018, at 17:34, Christophe Fergeau 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 signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide
> On 14 Feb 2018, at 17:34, Christophe Fergeau 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 Signed-off-by: Christophe de Dinechin --- .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 ..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 ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide
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 > > > > > > Signed-off-by: Christophe de Dinechin > > > --- > > > .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 ..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 signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide
> > 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 > > > > Signed-off-by: Christophe de Dinechin > > --- > > .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 ..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
Re: [Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide
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 > > Signed-off-by: Christophe de Dinechin > --- > .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 ..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 signature.asc Description: PGP signature ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v3 01/11] Add .clang-format with defaults matching what's specified in the style guide
From: Christophe de Dinechin Signed-off-by: Christophe de Dinechin --- .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 ..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
