Re: [Qemu-devel] Restore consistent formatting
On Wed, Feb 8, 2012 at 15:23, Anthony Liguori anth...@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 KR 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
On Wed, Feb 8, 2012 at 15:36, Andreas Färber afaer...@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
Am 11.02.2012 10:19, schrieb Blue Swirl: On Wed, Feb 8, 2012 at 15:36, Andreas Färber afaer...@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
Re: [Qemu-devel] Restore consistent formatting
Anthony Liguori anth...@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
On 02/09/2012 03:48 AM, Markus Armbruster wrote: Anthony Liguorianth...@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
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
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:av1...@comtv.ru
Re: [Qemu-devel] Restore consistent formatting
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
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
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
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
