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
