Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces
On Thu, Feb 08, 2018 at 11:09:35AM +0100, Victor Toso wrote: > Hi, > > On Thu, Feb 08, 2018 at 05:01:05AM -0500, Frediano Ziglio wrote: > > > > Depends on many cases. You don't want spurious changes to make harder to > > > > look at the history for instance (that is a point for Nack). > > > > The patch is not fixing anything or adding new feature (another for > > > > Nack). > > > > On the other hand applied to code not changed for a long period (where > > > > is > > > > unlikely to have to dig the history) or code with small history (where > > > > is faster to skip in any case) changes. > > > > Being style it depends also on personal opinions. > > > > > > You can’t have it both ways. Here, you are simultaneously saying: > > > > > > - We don’t want trailing whitespace > > > > OT: Not only trailing, also tabs for instance. > > > > > - We nack patches that fix trailing whitespace on their own > > > > Not saying that, I'm saying is not black and white. > > Because the code itself is inconsistent. It would be so much > better to have a few patches that make the code consistent and > then some git hook to check if given patch does not mess around > the coding style instead of discussing this so many times over > years. Yup, would be good to have... Regarding whitespace changes, I don't think we have many trailing whitespace problems in spice*, probably a bit more tabs here and there. Regarding fixing whitespace, I'll generally ack patches fixing one or 2 unrelated whitespace issues in a function the commit is modifying, or a patch fixing the whitespace issues in a file the patch series is going to change. Not sure what I would say to a patch fixing this over all the codebase (mostly depends on how big it would be :) 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 v2 12/13] Add guidelines about warnings and whitespaces
Hi, On Thu, Feb 08, 2018 at 11:14:02AM +0100, Christophe de Dinechin wrote: > > Because the code itself is inconsistent. It would be so much > > better to have a few patches that make the code consistent and > > then some git hook to check if given patch does not mess around > > the coding style instead of discussing this so many times over > > years. > > I thought I was requesting even less than a big fix. I was only > requesting to allow small fixes. But if Frediano does not want > this, then I would go with you and say we fix it once and then > enforce it. > > Christophe Could be something like intel-vaapi-driver, while fixing some small things, I was not able to commit due some hook that checked the file that I changed. So I did a single commit per file to make it all good and applied my changes as follow up. [0] https://github.com/intel/intel-vaapi-driver/commit/3443434d88ad116 Something like this should be fine for everybody, I think... toso 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 v2 12/13] Add guidelines about warnings and whitespaces
On Thu, 2018-02-08 at 11:09 +0100, Victor Toso wrote: > Hi, > > On Thu, Feb 08, 2018 at 05:01:05AM -0500, Frediano Ziglio wrote: > > > > Depends on many cases. You don't want spurious changes to make harder to > > > > look at the history for instance (that is a point for Nack). > > > > The patch is not fixing anything or adding new feature (another for > > > > Nack). > > > > On the other hand applied to code not changed for a long period (where > > > > is > > > > unlikely to have to dig the history) or code with small history (where > > > > is faster to skip in any case) changes. > > > > Being style it depends also on personal opinions. > > > > > > You can’t have it both ways. Here, you are simultaneously saying: > > > > > > - We don’t want trailing whitespace > > > > OT: Not only trailing, also tabs for instance. > > > > > - We nack patches that fix trailing whitespace on their own > > > > Not saying that, I'm saying is not black and white. > > Because the code itself is inconsistent. It would be so much > better to have a few patches that make the code consistent and > then some git hook to check if given patch does not mess around > the coding style instead of discussing this so many times over > years. Agreed! My (partly philosofical) opinion on this is: Code quality is more important than history (or diffs). If something is wrong, just go ahead and fix it. If you want it fixed, it will be recorded in the history. It's cleaner to have a separate commit for it in the history. (that is obvious, right?) Lukas ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces
> On 8 Feb 2018, at 11:09, Victor Toso wrote: > > Hi, > > On Thu, Feb 08, 2018 at 05:01:05AM -0500, Frediano Ziglio wrote: Depends on many cases. You don't want spurious changes to make harder to look at the history for instance (that is a point for Nack). The patch is not fixing anything or adding new feature (another for Nack). On the other hand applied to code not changed for a long period (where is unlikely to have to dig the history) or code with small history (where is faster to skip in any case) changes. Being style it depends also on personal opinions. >>> >>> You can’t have it both ways. Here, you are simultaneously saying: >>> >>> - We don’t want trailing whitespace >> >> OT: Not only trailing, also tabs for instance. >> >>> - We nack patches that fix trailing whitespace on their own >> >> Not saying that, I'm saying is not black and white. > > Because the code itself is inconsistent. It would be so much > better to have a few patches that make the code consistent and > then some git hook to check if given patch does not mess around > the coding style instead of discussing this so many times over > years. I thought I was requesting even less than a big fix. I was only requesting to allow small fixes. But if Frediano does not want this, then I would go with you and say we fix it once and then enforce it. Christophe > >toso > ___ > 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 v2 12/13] Add guidelines about warnings and whitespaces
> On 8 Feb 2018, at 11:01, Frediano Ziglio wrote: > >>> >>> On 8 Feb 2018, at 10:52, Frediano Ziglio wrote: >>> On Thu, 2018-02-08 at 10:21 +0100, Victor Toso wrote: > Hey, > > On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote: >> On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote: From: Christophe de Dinechin The objective of these guidelines is that: - We avoid introducing new warnings - We know how to fix old ones - We don't have to isolate whitespace changes when submitting patches, i.e. someone who use tools that automatically strip whitespaces and therefore "repairs" earlier errors should not be punished for it. >>> >>> Sorry, I don't agree with the automatic tool, patches should not >>> contain extra changes unless they fix space changes while changing >>> these lines for other reasons. >>> I'll personally accept single patches fixing the spaces. >> >> I'm with Frediano here. If you want to automatically fix whitespace >> errors, you can do it in a separate commit without much effort? >> >> I can also go right now and fix all trailing whitespaces with a bash >> oneliner, submit the patch and we have a moot point here? :) > > Well, it was nacked before :) > > https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html What is the reasoning behind the nack? I'd rather have a style-only commit than to fight the bad indentation manually and possibly have unrelated whitespace changes in another patch... By the way, mine got acked :D https://lists.freedesktop.org/archives/spice-devel/2018-January/041527.html Lukas >>> >>> Depends on many cases. You don't want spurious changes to make harder to >>> look at the history for instance (that is a point for Nack). >>> The patch is not fixing anything or adding new feature (another for Nack). >>> On the other hand applied to code not changed for a long period (where is >>> unlikely to have to dig the history) or code with small history (where >>> is faster to skip in any case) changes. >>> Being style it depends also on personal opinions. >> >> You can’t have it both ways. Here, you are simultaneously saying: >> >> - We don’t want trailing whitespace > > OT: Not only trailing, also tabs for instance. > >> - We nack patches that fix trailing whitespace on their own > > Not saying that, I'm saying is not black and white. Granted, you are not saying it, Marc-André is… But if one nacks patches with only whitespace and the other nacks patches where whitespace are incidental, we still have a problem. > >> - We nack patches that include incidental whitespace fixes >> > > Not saying that either, I'm saying that if the patch says "fix this" > and is also fixing spaces in unrelated hunks should be split. Which is exactly what I am saying, isn’t it? > >> Sorry, I can’t agree with that, it’s inconsistent. >> >> >> Christophe >> > > 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
Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces
> On 8 Feb 2018, at 10:02, Frediano Ziglio wrote: > >> >> From: Christophe de Dinechin >> >> The objective of these guidelines is that: >> - We avoid introducing new warnings >> - We know how to fix old ones >> - We don't have to isolate whitespace changes when submitting patches, >> i.e. someone who use tools that automatically strip whitespaces and >> therefore "repairs" earlier errors should not be punished for it. > > Sorry, I don't agree with the automatic tool, patches should not > contain extra changes unless they fix space changes while changing > these lines for other reasons. > I'll personally accept single patches fixing the spaces. But as Victor pointed out, others don’t. > >> >> Signed-off-by: Christophe de Dinechin >> --- >> docs/spice_style.txt | 9 + >> 1 file changed, 9 insertions(+) >> >> diff --git a/docs/spice_style.txt b/docs/spice_style.txt >> index e2465aa9..d9644f9a 100644 >> --- a/docs/spice_style.txt >> +++ b/docs/spice_style.txt >> @@ -404,3 +404,12 @@ Also in source (no header) files you must include >> `config.h` at the beginning so >> >> #include "spice_server.h" >> >> + >> + >> +Compilation >> +--- >> + >> +The source code should compile without warnings on all variants of GCC and >> clang available. > > This is quite strong, looks like before sending a patch you should > use any variant you find. That was not the intent, which is why it says “should” and not “must”. > > I would go with a more open specification not including the compilers: > > "The source code should compile without warnings” But that’s even stronger. It means on any compiler… > >> +A patch may be rejected if it introduces new warnings. >> +Warnings that appear over time due to improvements in compilers should be >> fixed in dedicated patches. A patch should not mix warning fixes and other >> changes. > > agreed > >> +Any patch may adjust whitespace (e.g. eliminate trailing whitespace). >> Whitespace adjustments do not require specific patches. > > don't agree Impasse then, because I care about this one based on observing the history of whitespace-only changes being nacked and whitespace-including changes being also nacked. > > Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces
Hi, On Thu, Feb 08, 2018 at 05:01:05AM -0500, Frediano Ziglio wrote: > > > Depends on many cases. You don't want spurious changes to make harder to > > > look at the history for instance (that is a point for Nack). > > > The patch is not fixing anything or adding new feature (another for Nack). > > > On the other hand applied to code not changed for a long period (where is > > > unlikely to have to dig the history) or code with small history (where > > > is faster to skip in any case) changes. > > > Being style it depends also on personal opinions. > > > > You can’t have it both ways. Here, you are simultaneously saying: > > > > - We don’t want trailing whitespace > > OT: Not only trailing, also tabs for instance. > > > - We nack patches that fix trailing whitespace on their own > > Not saying that, I'm saying is not black and white. Because the code itself is inconsistent. It would be so much better to have a few patches that make the code consistent and then some git hook to check if given patch does not mess around the coding style instead of discussing this so many times over years. toso 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 v2 12/13] Add guidelines about warnings and whitespaces
> > > On 8 Feb 2018, at 10:52, Frediano Ziglio wrote: > > > >> > >> On Thu, 2018-02-08 at 10:21 +0100, Victor Toso wrote: > >>> Hey, > >>> > >>> On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote: > On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote: > >> > >> From: Christophe de Dinechin > >> > >> The objective of these guidelines is that: > >> - We avoid introducing new warnings > >> - We know how to fix old ones > >> - We don't have to isolate whitespace changes when submitting > >> patches, > >> i.e. someone who use tools that automatically strip whitespaces and > >> therefore "repairs" earlier errors should not be punished for it. > > > > Sorry, I don't agree with the automatic tool, patches should not > > contain extra changes unless they fix space changes while changing > > these lines for other reasons. > > I'll personally accept single patches fixing the spaces. > > I'm with Frediano here. If you want to automatically fix whitespace > errors, you can do it in a separate commit without much effort? > > I can also go right now and fix all trailing whitespaces with a bash > oneliner, submit the patch and we have a moot point here? :) > >>> > >>> Well, it was nacked before :) > >>> > >>> https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html > >> > >> What is the reasoning behind the nack? > >> > >> I'd rather have a style-only commit than to fight the bad indentation > >> manually and possibly have unrelated whitespace changes in another > >> patch... > >> > >> By the way, mine got acked :D > >> > >> https://lists.freedesktop.org/archives/spice-devel/2018-January/041527.html > >> > >> Lukas > >> > > > > Depends on many cases. You don't want spurious changes to make harder to > > look at the history for instance (that is a point for Nack). > > The patch is not fixing anything or adding new feature (another for Nack). > > On the other hand applied to code not changed for a long period (where is > > unlikely to have to dig the history) or code with small history (where > > is faster to skip in any case) changes. > > Being style it depends also on personal opinions. > > You can’t have it both ways. Here, you are simultaneously saying: > > - We don’t want trailing whitespace OT: Not only trailing, also tabs for instance. > - We nack patches that fix trailing whitespace on their own Not saying that, I'm saying is not black and white. > - We nack patches that include incidental whitespace fixes > Not saying that either, I'm saying that if the patch says "fix this" and is also fixing spaces in unrelated hunks should be split. > Sorry, I can’t agree with that, it’s inconsistent. > > > Christophe > Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces
> On 8 Feb 2018, at 10:52, Frediano Ziglio wrote: > >> >> On Thu, 2018-02-08 at 10:21 +0100, Victor Toso wrote: >>> Hey, >>> >>> On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote: On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote: >> >> From: Christophe de Dinechin >> >> The objective of these guidelines is that: >> - We avoid introducing new warnings >> - We know how to fix old ones >> - We don't have to isolate whitespace changes when submitting >> patches, >> i.e. someone who use tools that automatically strip whitespaces and >> therefore "repairs" earlier errors should not be punished for it. > > Sorry, I don't agree with the automatic tool, patches should not > contain extra changes unless they fix space changes while changing > these lines for other reasons. > I'll personally accept single patches fixing the spaces. I'm with Frediano here. If you want to automatically fix whitespace errors, you can do it in a separate commit without much effort? I can also go right now and fix all trailing whitespaces with a bash oneliner, submit the patch and we have a moot point here? :) >>> >>> Well, it was nacked before :) >>> >>> https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html >> >> What is the reasoning behind the nack? >> >> I'd rather have a style-only commit than to fight the bad indentation >> manually and possibly have unrelated whitespace changes in another >> patch... >> >> By the way, mine got acked :D >> >> https://lists.freedesktop.org/archives/spice-devel/2018-January/041527.html >> >> Lukas >> > > Depends on many cases. You don't want spurious changes to make harder to > look at the history for instance (that is a point for Nack). > The patch is not fixing anything or adding new feature (another for Nack). > On the other hand applied to code not changed for a long period (where is > unlikely to have to dig the history) or code with small history (where > is faster to skip in any case) changes. > Being style it depends also on personal opinions. You can’t have it both ways. Here, you are simultaneously saying: - We don’t want trailing whitespace - We nack patches that fix trailing whitespace on their own - We nack patches that include incidental whitespace fixes Sorry, I can’t agree with that, it’s inconsistent. Christophe > > Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces
> > On Thu, 2018-02-08 at 10:21 +0100, Victor Toso wrote: > > Hey, > > > > On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote: > > > On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote: > > > > > > > > > > From: Christophe de Dinechin > > > > > > > > > > The objective of these guidelines is that: > > > > > - We avoid introducing new warnings > > > > > - We know how to fix old ones > > > > > - We don't have to isolate whitespace changes when submitting > > > > > patches, > > > > > i.e. someone who use tools that automatically strip whitespaces and > > > > > therefore "repairs" earlier errors should not be punished for it. > > > > > > > > Sorry, I don't agree with the automatic tool, patches should not > > > > contain extra changes unless they fix space changes while changing > > > > these lines for other reasons. > > > > I'll personally accept single patches fixing the spaces. > > > > > > I'm with Frediano here. If you want to automatically fix whitespace > > > errors, you can do it in a separate commit without much effort? > > > > > > I can also go right now and fix all trailing whitespaces with a bash > > > oneliner, submit the patch and we have a moot point here? :) > > > > Well, it was nacked before :) > > > > https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html > > What is the reasoning behind the nack? > > I'd rather have a style-only commit than to fight the bad indentation > manually and possibly have unrelated whitespace changes in another > patch... > > By the way, mine got acked :D > > https://lists.freedesktop.org/archives/spice-devel/2018-January/041527.html > > Lukas > Depends on many cases. You don't want spurious changes to make harder to look at the history for instance (that is a point for Nack). The patch is not fixing anything or adding new feature (another for Nack). On the other hand applied to code not changed for a long period (where is unlikely to have to dig the history) or code with small history (where is faster to skip in any case) changes. Being style it depends also on personal opinions. Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces
On Thu, 2018-02-08 at 10:21 +0100, Victor Toso wrote: > Hey, > > On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote: > > On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote: > > > > > > > > From: Christophe de Dinechin > > > > > > > > The objective of these guidelines is that: > > > > - We avoid introducing new warnings > > > > - We know how to fix old ones > > > > - We don't have to isolate whitespace changes when submitting patches, > > > > i.e. someone who use tools that automatically strip whitespaces and > > > > therefore "repairs" earlier errors should not be punished for it. > > > > > > Sorry, I don't agree with the automatic tool, patches should not > > > contain extra changes unless they fix space changes while changing > > > these lines for other reasons. > > > I'll personally accept single patches fixing the spaces. > > > > I'm with Frediano here. If you want to automatically fix whitespace > > errors, you can do it in a separate commit without much effort? > > > > I can also go right now and fix all trailing whitespaces with a bash > > oneliner, submit the patch and we have a moot point here? :) > > Well, it was nacked before :) > > https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html What is the reasoning behind the nack? I'd rather have a style-only commit than to fight the bad indentation manually and possibly have unrelated whitespace changes in another patch... By the way, mine got acked :D https://lists.freedesktop.org/archives/spice-devel/2018-January/041527.html Lukas ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces
Hey, On Thu, Feb 08, 2018 at 10:13:21AM +0100, Lukáš Hrázký wrote: > On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote: > > > > > > From: Christophe de Dinechin > > > > > > The objective of these guidelines is that: > > > - We avoid introducing new warnings > > > - We know how to fix old ones > > > - We don't have to isolate whitespace changes when submitting patches, > > > i.e. someone who use tools that automatically strip whitespaces and > > > therefore "repairs" earlier errors should not be punished for it. > > > > Sorry, I don't agree with the automatic tool, patches should not > > contain extra changes unless they fix space changes while changing > > these lines for other reasons. > > I'll personally accept single patches fixing the spaces. > > I'm with Frediano here. If you want to automatically fix whitespace > errors, you can do it in a separate commit without much effort? > > I can also go right now and fix all trailing whitespaces with a bash > oneliner, submit the patch and we have a moot point here? :) Well, it was nacked before :) https://lists.freedesktop.org/archives/spice-devel/2016-May/029603.html 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 v2 12/13] Add guidelines about warnings and whitespaces
On Thu, 2018-02-08 at 04:02 -0500, Frediano Ziglio wrote: > > > > From: Christophe de Dinechin > > > > The objective of these guidelines is that: > > - We avoid introducing new warnings > > - We know how to fix old ones > > - We don't have to isolate whitespace changes when submitting patches, > > i.e. someone who use tools that automatically strip whitespaces and > > therefore "repairs" earlier errors should not be punished for it. > > Sorry, I don't agree with the automatic tool, patches should not > contain extra changes unless they fix space changes while changing > these lines for other reasons. > I'll personally accept single patches fixing the spaces. I'm with Frediano here. If you want to automatically fix whitespace errors, you can do it in a separate commit without much effort? I can also go right now and fix all trailing whitespaces with a bash oneliner, submit the patch and we have a moot point here? :) Lukas > > > > Signed-off-by: Christophe de Dinechin > > --- > > docs/spice_style.txt | 9 + > > 1 file changed, 9 insertions(+) > > > > diff --git a/docs/spice_style.txt b/docs/spice_style.txt > > index e2465aa9..d9644f9a 100644 > > --- a/docs/spice_style.txt > > +++ b/docs/spice_style.txt > > @@ -404,3 +404,12 @@ Also in source (no header) files you must include > > `config.h` at the beginning so > > > > #include "spice_server.h" > > > > + > > + > > +Compilation > > +--- > > + > > +The source code should compile without warnings on all variants of GCC and > > clang available. > > This is quite strong, looks like before sending a patch you should > use any variant you find. > > I would go with a more open specification not including the compilers: > > "The source code should compile without warnings" > > > +A patch may be rejected if it introduces new warnings. > > +Warnings that appear over time due to improvements in compilers should be > > fixed in dedicated patches. A patch should not mix warning fixes and other > > changes. > > agreed > > > +Any patch may adjust whitespace (e.g. eliminate trailing whitespace). > > Whitespace adjustments do not require specific patches. > > don't agree > > 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
Re: [Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces
> > From: Christophe de Dinechin > > The objective of these guidelines is that: > - We avoid introducing new warnings > - We know how to fix old ones > - We don't have to isolate whitespace changes when submitting patches, > i.e. someone who use tools that automatically strip whitespaces and > therefore "repairs" earlier errors should not be punished for it. Sorry, I don't agree with the automatic tool, patches should not contain extra changes unless they fix space changes while changing these lines for other reasons. I'll personally accept single patches fixing the spaces. > > Signed-off-by: Christophe de Dinechin > --- > docs/spice_style.txt | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/docs/spice_style.txt b/docs/spice_style.txt > index e2465aa9..d9644f9a 100644 > --- a/docs/spice_style.txt > +++ b/docs/spice_style.txt > @@ -404,3 +404,12 @@ Also in source (no header) files you must include > `config.h` at the beginning so > > #include "spice_server.h" > > + > + > +Compilation > +--- > + > +The source code should compile without warnings on all variants of GCC and > clang available. This is quite strong, looks like before sending a patch you should use any variant you find. I would go with a more open specification not including the compilers: "The source code should compile without warnings" > +A patch may be rejected if it introduces new warnings. > +Warnings that appear over time due to improvements in compilers should be > fixed in dedicated patches. A patch should not mix warning fixes and other > changes. agreed > +Any patch may adjust whitespace (e.g. eliminate trailing whitespace). > Whitespace adjustments do not require specific patches. don't agree Frediano ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
[Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces
From: Christophe de Dinechin The objective of these guidelines is that: - We avoid introducing new warnings - We know how to fix old ones - We don't have to isolate whitespace changes when submitting patches, i.e. someone who use tools that automatically strip whitespaces and therefore "repairs" earlier errors should not be punished for it. Signed-off-by: Christophe de Dinechin --- docs/spice_style.txt | 9 + 1 file changed, 9 insertions(+) diff --git a/docs/spice_style.txt b/docs/spice_style.txt index e2465aa9..d9644f9a 100644 --- a/docs/spice_style.txt +++ b/docs/spice_style.txt @@ -404,3 +404,12 @@ Also in source (no header) files you must include `config.h` at the beginning so #include "spice_server.h" + + +Compilation +--- + +The source code should compile without warnings on all variants of GCC and clang available. +A patch may be rejected if it introduces new warnings. +Warnings that appear over time due to improvements in compilers should be fixed in dedicated patches. A patch should not mix warning fixes and other changes. +Any patch may adjust whitespace (e.g. eliminate trailing whitespace). Whitespace adjustments do not require specific patches. -- 2.13.5 (Apple Git-94) ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/spice-devel
