Re: [Qemu-devel] Restore consistent formatting

From: Anthony Liguori <anthony@codemonkey.ws>
To: "Andreas Färber" <afaerber@suse.de>
Cc: Blue Swirl <blauwirbel@gmail.com>,
	Stefan Hajnoczi <stefanha@gmail.com>,
	qemu-devel@nongnu.org, Evgeny Voevodin <e.voevodin@samsung.com>,
	Dmitry Solodkiy <d.solodkiy@samsung.com>
Subject: Re: [Qemu-devel] Restore consistent formatting
Date: Wed, 08 Feb 2012 09:48:33 -0600	[thread overview]
Message-ID: <4F329951.5060506@codemonkey.ws> (raw)
In-Reply-To: <4F329660.7040101@suse.de>

On 02/08/2012 09:36 AM, Andreas Färber wrote:
> Am 08.02.2012 16:23, schrieb Anthony Liguori:
>> On 02/08/2012 09:04 AM, malc wrote:
>>> On Wed, 8 Feb 2012, Andreas F?rber wrote:
>>>
>>>> Arbitrarily reformatting your files is not okay. If you want a different
>>>> formatting, you need to fix checkpatch.pl first to not error on that
>>>> formatting in your files.
>>>
>>> It was always formatter like this (internally consistent), then others
>>> added code which made it not so.
>>
>> We do have a mixed style in the audio layer.  I'm not happy about that
>> but I also feel strongly that going through and doing a reformat is not
>> a worthwhile exercise.
>>
>> I can also understand the desire to keep things consistent.  But patches
>> should always go to the mailing list.  I certainly would have acked such
>> a patch FWIW.
>>
>> I think people get a bit too excited about coding style.  There are much
>> more important things to worry about in life than the number of spaces
>> before a parenthesis :-)
>
> This is not about whether or not we put a space somewhere.
>
> It's about reviewers and SubmitAPatch telling people to run
> checkpatch.pl on patches and checkpatch.pl reporting this as an ERROR,
> not a WARNING. So if you follow Stefan's instructions on running the
> script as a commit hook (which is the only sane way to run it when
> handling lots of patches) you can't commit a patch or your local changes
> when there are ERRORs.

It's a suggestion, it still assumes that you are going to exercise discretion 
and make rational decisions when checkpatch does something silly.

>
> I just spent half the night trying to find out why checkpatch.pl reports
> CPUX86State *env, CPUYState *env, CPyState *env as ERRORs but not
> CPUState *env. I did not succeed in really understanding it.
>
> So either we need to all stop using and telling to use checkpatch.pl or
> someone needs to fix it.

checkpatch.pl is a tool.  Tools are meant to make our lives easier, not harder. 
  You should use checkpatch.pl to help you figure out if you have coding style 
issues but it is not a QEMU maintainer that gives you a required Ack before you 
code gets accepted.  If it's doing something stupid, ignore it.

Making checkpatch 100% perfect (or event 99% perfect) is simply not worth the 
effort.  Parsing C is insanely hard and doing it in perl only makes the problem 
worse :-)

If you want to tone down the language in SubmitAPatch, please go ahead.  It's a 
wiki after all.

Regards,

Anthony Liguori

>
> Andreas
>