[Qemu-devel] Restore consistent formatting

[Qemu-devel] Restore consistent formatting

[Evgeny Voevodin]

The last commit "Restore consistent formatting" 
(cf4dc461a4cfc3e056ee24edb26154f4d34a6278) which is on the master but 
absent in mailing list results in such output of checkpatch.pl:

./scripts/checkpatch.pl --no-tree --file hw/cs4231a.c
...
total: 107 errors, 88 warnings, 696 lines checked

-----------------------------------------------------------------

./scripts/checkpatch.pl --no-tree --file hw/adlib.c
...
total: 7 errors, 58 warnings, 337 lines checked

-----------------------------------------------------------------

./scripts/checkpatch.pl --no-tree --file hw/ac97.c
...
total: 16 errors, 324 warnings, 1380 lines checked


and so on.
-- 
Kind regards,
Evgeny Voevodin,
Leading Software Engineer,
ASWG, Moscow R&D center, Samsung Electronics
e-mail: e.voevodin@samsung.com

Re: [Qemu-devel] Restore consistent formatting

[Andreas Färber]

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.

And please post your patches to the mailing list.

Thanks,
Andreas

Am 08.02.2012 11:11, schrieb Evgeny Voevodin:
> The last commit "Restore consistent formatting"
> (cf4dc461a4cfc3e056ee24edb26154f4d34a6278) which is on the master but
> absent in mailing list results in such output of checkpatch.pl:
> 
> ./scripts/checkpatch.pl --no-tree --file hw/cs4231a.c
> ...
> total: 107 errors, 88 warnings, 696 lines checked
> 
> -----------------------------------------------------------------
> 
> ./scripts/checkpatch.pl --no-tree --file hw/adlib.c
> ...
> total: 7 errors, 58 warnings, 337 lines checked
> 
> -----------------------------------------------------------------
> 
> ./scripts/checkpatch.pl --no-tree --file hw/ac97.c
> ...
> total: 16 errors, 324 warnings, 1380 lines checked
> 
> 
> and so on.


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] Restore consistent formatting

[malc]

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.

[..snip..]

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] Restore consistent formatting

[Anthony Liguori]

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.

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 :-)

Regards,

Anthony Liguori

>
> [..snip..]
>

Re: [Qemu-devel] Restore consistent formatting

[Andreas Färber]

Am 08.02.2012 16:04, schrieb malc:
> 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.

That's not the point. We use the checkpatch.pl script to check the
formatting of patches, we tell contributors to run it. If you want your
files to have an additional space then _you_ would have to change the
script to not error on that formatting in certain files or else your
formatting changes will get reverted again in the parts other people touch.

Right now you silently caused a needless conflict with other people's
patches, including device_init() -> type_init().

That's a really great way to say "I'm back" after lots of 'mail
receiving disabled' messages...

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] Restore consistent formatting

[Andreas Färber]

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

Re: [Qemu-devel] Restore consistent formatting

[Anthony Liguori]

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
>

Re: [Qemu-devel] Restore consistent formatting

[Markus Armbruster]

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.

[...]

Re: [Qemu-devel] Restore consistent formatting

[Anthony Liguori]

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


>
> [...]
>

Re: [Qemu-devel] Restore consistent formatting

[Blue Swirl]

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..]
>>
>

Re: [Qemu-devel] Restore consistent formatting

[Blue Swirl]

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

Re: [Qemu-devel] Restore consistent formatting

[Andreas Färber]

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