|
From: | Anthony Liguori |
Subject: | Re: [Qemu-devel] Restore consistent formatting |
Date: | Wed, 08 Feb 2012 09:48:33 -0600 |
User-agent: | Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.23) Gecko/20110922 Lightning/1.0b2 Thunderbird/3.1.15 |
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
[Prev in Thread] | Current Thread | [Next in Thread] |