* Re: [Qemu-devel] Restore consistent formatting
2012-02-08 15:23 ` Anthony Liguori
@ 2012-02-08 15:36 ` Andreas Färber
2012-02-08 15:48 ` Anthony Liguori
2012-02-11 9:19 ` Blue Swirl
2012-02-09 9:48 ` Markus Armbruster
2012-02-11 9:05 ` Blue Swirl
2 siblings, 2 replies; 12+ messages in thread
From: Andreas Färber @ 2012-02-08 15:36 UTC (permalink / raw)
To: Anthony Liguori
Cc: Evgeny Voevodin, Stefan Hajnoczi, qemu-devel, Blue Swirl,
Dmitry Solodkiy
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.
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.
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] Restore consistent formatting
2012-02-08 15:36 ` Andreas Färber
@ 2012-02-08 15:48 ` Anthony Liguori
2012-02-11 9:19 ` Blue Swirl
1 sibling, 0 replies; 12+ messages in thread
From: Anthony Liguori @ 2012-02-08 15:48 UTC (permalink / raw)
To: Andreas Färber
Cc: Blue Swirl, Stefan Hajnoczi, qemu-devel, Evgeny Voevodin,
Dmitry Solodkiy
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] Restore consistent formatting
2012-02-08 15:36 ` Andreas Färber
2012-02-08 15:48 ` Anthony Liguori
@ 2012-02-11 9:19 ` Blue Swirl
2012-02-11 12:18 ` Andreas Färber
1 sibling, 1 reply; 12+ messages in thread
From: Blue Swirl @ 2012-02-11 9:19 UTC (permalink / raw)
To: Andreas Färber
Cc: Evgeny Voevodin, Stefan Hajnoczi, qemu-devel, Dmitry Solodkiy
On Wed, Feb 8, 2012 at 15:36, Andreas Färber <afaerber@suse.de> 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.
The warning levels are from Linux, they could well be incorrect.
> 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.
Maybe the CPUState #defines in target-*/cpu.h confuse checkpatch.
> So either we need to all stop using and telling to use checkpatch.pl or
> someone needs to fix it.
It's not that black or white. Obviously checkpatch.pl can make
mistakes and some of its rules are actually not based on CODING_STYLE
but they are in line with the current code (except for example in this
audio case), so the output is usually helpful to make code more
uniform. I wouldn't "fix" checkpatch to consider local style
variations. If we don't want a rule that complains spaces after
function identifiers, it could be removed entirely. Though then
someone could submit patches to introduce them elsewhere, but if there
is no rule, that should be OK.
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] Restore consistent formatting
2012-02-11 9:19 ` Blue Swirl
@ 2012-02-11 12:18 ` Andreas Färber
0 siblings, 0 replies; 12+ messages in thread
From: Andreas Färber @ 2012-02-11 12:18 UTC (permalink / raw)
To: Blue Swirl; +Cc: Evgeny Voevodin, Stefan Hajnoczi, qemu-devel, Dmitry Solodkiy
Am 11.02.2012 10:19, schrieb Blue Swirl:
> On Wed, Feb 8, 2012 at 15:36, Andreas Färber <afaerber@suse.de> wrote:
>> 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.
>
> The warning levels are from Linux, they could well be incorrect.
My memory tricked me, I must've mixed it up with the other issue I was
investigating (too little sleep and very annoyed about the monolithic
fragments of Perl source code and checkpatch.pl not conforming to its
own rules, e.g. tab indention and > 80 chars).
Still, my point remains: what shows up in checkpatch.pl output will get
fixed by new contributors, whether WARNING or ERROR.
And checkpatch.pl cannot distinguish between changes that correctly
update old-style code (e.g., adding braces for if) and code that aims
for consistency in a file.
Leaving checkpatch.pl unchanged and demanding this special case doesn't
go along well and will cause frustration on all sides and reformatting
back and forth, something that was declared forbidden elsewhere and
patches doing reformattings were turned down.
Andreas
P.S. For the record, Stefan pointed me to git-commit --no-verify for
ignoring checkpatch ERRORs.
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] Restore consistent formatting
2012-02-08 15:23 ` Anthony Liguori
2012-02-08 15:36 ` Andreas Färber
@ 2012-02-09 9:48 ` Markus Armbruster
2012-02-09 13:46 ` Anthony Liguori
2012-02-11 9:05 ` Blue Swirl
2 siblings, 1 reply; 12+ messages in thread
From: Markus Armbruster @ 2012-02-09 9:48 UTC (permalink / raw)
To: Anthony Liguori
Cc: Evgeny Voevodin, qemu-devel, Blue Swirl, Dmitry Solodkiy,
Andreas Färber
Anthony Liguori <anthony@codemonkey.ws> writes:
> On 02/08/2012 09:04 AM, malc wrote:
>> On Wed, 8 Feb 2012, Andreas F?rber wrote:
>>
>>> malc,
>>>
>>> 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.
You buried the one truly important sentence, let me dig it out for you:
*** Patches should always go to the mailing list ***
Exceptions need justification. Responsible handling embargoed security
issues may qualify. Style fixes certainly not.
[...]
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Qemu-devel] Restore consistent formatting
2012-02-09 9:48 ` Markus Armbruster
@ 2012-02-09 13:46 ` Anthony Liguori
0 siblings, 0 replies; 12+ messages in thread
From: Anthony Liguori @ 2012-02-09 13:46 UTC (permalink / raw)
To: Markus Armbruster
Cc: Blue Swirl, Andreas Färber, qemu-devel, Evgeny Voevodin,
Dmitry Solodkiy
On 02/09/2012 03:48 AM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws> writes:
>
>> On 02/08/2012 09:04 AM, malc wrote:
>>> On Wed, 8 Feb 2012, Andreas F?rber wrote:
>>>
>>>> malc,
>>>>
>>>> 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.
>
> You buried the one truly important sentence, let me dig it out for you:
>
> *** Patches should always go to the mailing list ***
>
> Exceptions need justification. Responsible handling embargoed security
> issues may qualify. Style fixes certainly not.
100% agreed.
Regards,
Anthony Liguori
>
> [...]
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Qemu-devel] Restore consistent formatting
2012-02-08 15:23 ` Anthony Liguori
2012-02-08 15:36 ` Andreas Färber
2012-02-09 9:48 ` Markus Armbruster
@ 2012-02-11 9:05 ` Blue Swirl
2 siblings, 0 replies; 12+ messages in thread
From: Blue Swirl @ 2012-02-11 9:05 UTC (permalink / raw)
To: Anthony Liguori
Cc: Dmitry Solodkiy, Andreas Färber, Evgeny Voevodin, qemu-devel
On Wed, Feb 8, 2012 at 15:23, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 02/08/2012 09:04 AM, malc wrote:
>>
>> On Wed, 8 Feb 2012, Andreas F?rber wrote:
>>
>>> malc,
>>>
>>> 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.
Really? First, this would imply that local consistency is more
important to you than global consistency and second, pure reformatting
patches are now acceptable to you (despite previous git blame breakage
reasoning).
What would your reaction be if I declared that CODING_STYLE for
target-sparc/* is Linux kernel one (some pieces could probably be
found which would comply even now to make the precedent case for
argument's sake) and post a reformatting patch to the list?
> 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 :-)
In general, a coding style is one aspect of development guidance. The
level of guidance can vary, it could be missing, there could be a
loose convention, a guideline or even a iron hard rule. It could apply
differently case by case or globally.
In case of coding style, I'd strongly prefer that the level of
guidance is clear to everyone and it doesn't change very often. As to
the actual content of the guidance, that can never be precise.
In this specific case, CODING_STYLE says nothing about spaces after
function identifiers. However, vast majority of code omits them.
Spaces are used in GNU style, not in K&R or Linux style and I think
QEMU style is closer to them than to GNU style. I'd say this case is
not unlike the previous cases where braces were added to code where
there were none before, so reformatting is going backwards (unless
local consistency is given priority).
Personally I prefer strong, enforced rules to loose conventions but
it's more important to me that everybody in a project plays by the
same rules (or agrees to lack of them), whatever they are. Personally
I also hate GNU style and would prefer that malc would swallow his
pride and reformat audio etc. to match rest of the world, but I
respect his status as taking responsibility for maintaining the audio
layer.
> Regards,
>
> Anthony Liguori
>
>>
>> [..snip..]
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread