[Spice-devel] [PATCH v2 12/13] Add guidelines about warnings and whitespaces

[Christophe de Dinechin]

> On 8 Feb 2018, at 10:02, Frediano Ziglio <fziglio at redhat.com> wrote:
> 
>> 
>> From: Christophe de Dinechin <dinechin at 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.
> 
> 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 <dinechin at 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 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