[v3,10/11] Add guidelines about warnings and whitespaces

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

Details

Message ID 20180208112531.19601-11-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>

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 <dinechin@redhat.com>
---
 docs/spice_style.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

Patch hide | download patch | download mbox

diff --git a/docs/spice_style.txt b/docs/spice_style.txt
index ae91f987..108a57a5 100644
--- a/docs/spice_style.txt
+++ b/docs/spice_style.txt
@@ -436,3 +436,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.

Comments

On Thu, Feb 08, 2018 at 12:25:30PM +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> 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 <dinechin@redhat.com>
> ---
>  docs/spice_style.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> index ae91f987..108a57a5 100644
> --- a/docs/spice_style.txt
> +++ b/docs/spice_style.txt
> @@ -436,3 +436,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.

I believe this part was quite controversial, so I'd drop it for now. To
be honest, the whole patch does not seem very useful to me, in my
opinion it's mostly stating the obvious.

Christophe
> On 14 Feb 2018, at 14:37, Christophe Fergeau <cfergeau@redhat.com> wrote:
> 
> On Thu, Feb 08, 2018 at 12:25:30PM +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> 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 <dinechin@redhat.com>
>> ---
>> docs/spice_style.txt | 9 +++++++++
>> 1 file changed, 9 insertions(+)
>> 
>> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
>> index ae91f987..108a57a5 100644
>> --- a/docs/spice_style.txt
>> +++ b/docs/spice_style.txt
>> @@ -436,3 +436,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.
> 
> I believe this part was quite controversial, so I'd drop it for now. To
> be honest, the whole patch does not seem very useful to me, in my
> opinion it's mostly stating the obvious.

I wish :-) Also, puzzled by the patch being “controversial” yet “stating the obvious”?


> 
> Christophe
On Wed, Feb 14, 2018 at 10:53:04PM +0100, Christophe de Dinechin wrote:
> 
> 
> > On 14 Feb 2018, at 14:37, Christophe Fergeau <cfergeau@redhat.com> wrote:
> > 
> > On Thu, Feb 08, 2018 at 12:25:30PM +0100, Christophe de Dinechin wrote:
> >> From: Christophe de Dinechin <dinechin@redhat.com>
> >> 
> >> 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 <dinechin@redhat.com>
> >> ---
> >> docs/spice_style.txt | 9 +++++++++
> >> 1 file changed, 9 insertions(+)
> >> 
> >> diff --git a/docs/spice_style.txt b/docs/spice_style.txt
> >> index ae91f987..108a57a5 100644
> >> --- a/docs/spice_style.txt
> >> +++ b/docs/spice_style.txt
> >> @@ -436,3 +436,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.
> > 
> > I believe this part was quite controversial, so I'd drop it for now. To
> > be honest, the whole patch does not seem very useful to me, in my
> > opinion it's mostly stating the obvious.
> 
> I wish :-) Also, puzzled by the patch being “controversial” yet “stating the obvious”?

The whitespace bits were controversial, the "compile without warnings"
part is in my opinion quite obivous.

Christophe