| Message ID | 1328799563-18932-1-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On Thu, 9 Feb 2012, Andreas F?rber wrote: > Disable warnings for spaces before opening parenthesis in > hw/{ac97,adlib,cs4231a,es1370,gus,sb16}.c. > > Signed-off-by: Andreas F?rber <afaerber@suse.de> > Cc: Blue Swirl <blauwirbel@gmail.com> > Cc: malc <av1474@comtv.ru> Thanks. > --- > scripts/checkpatch.pl | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 8850a5f..5433736 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -246,6 +246,8 @@ our $allowed_asm_includes = qr{(?x: > )}; > # memory.h: ARM has a custom one > > +our $audio_files = qr{hw/ac97.c|hw/adlib.c|hw/cs4231a.c|hw/es1370.c|hw/gus.c|hw/sb16.c}; My perl is not up to snuff so i wouldn't know how to add audio/* to the mix. > + > sub build_types { > my $mods = "(?x: \n" . join("|\n ", @modifierList) . "\n)"; > my $all = "(?x: \n" . join("|\n ", @typeList) . "\n)"; > @@ -1966,6 +1968,9 @@ sub process { > asm|__asm__)$/x) > { > > + # malc wants to keep spacing consistent in the audio files. > + } elsif ($realfile =~ /($audio_files)/) { > + > # cpp #define statements have non-optional spaces, ie > # if there is a space between the name and the open > # parenthesis it is simply not a parameter group. >
On 02/09/2012 06:59 PM, Andreas Färber wrote: > Disable warnings for spaces before opening parenthesis in > hw/{ac97,adlib,cs4231a,es1370,gus,sb16}.c. Why audio files are such a special thing? Isn't it be better to revert a patch that introduced checkpatch.pl errors?
On Fri, 10 Feb 2012, Evgeny Voevodin wrote: > On 02/09/2012 06:59 PM, Andreas F?rber wrote: > > Disable warnings for spaces before opening parenthesis in > > hw/{ac97,adlib,cs4231a,es1370,gus,sb16}.c. > > Why audio files are such a special thing? Because they are consistently formatted the way they are. > Isn't it be better to revert a patch that introduced checkpatch.pl errors? No.
On 02/09/2012 10:02 PM, malc wrote: > On Fri, 10 Feb 2012, Evgeny Voevodin wrote: > >> On 02/09/2012 06:59 PM, Andreas F?rber wrote: >>> Disable warnings for spaces before opening parenthesis in >>> hw/{ac97,adlib,cs4231a,es1370,gus,sb16}.c. >> >> Why audio files are such a special thing? > > Because they are consistently formatted the way they are. I personally hate the QEMU Coding Style I dislike inconsistency more than any particular style. So I'm with malc here. I'd be opposed to introducing a new file that deviated from Coding Style but for the ones that already do, I see no reason to convert them all at once or make the code deviate from the style it's already using. > >> Isn't it be better to revert a patch that introduced checkpatch.pl errors? > > No. Regards, Anthony Liguori >
On Thu, Feb 9, 2012 at 14:59, Andreas Färber <afaerber@suse.de> wrote: > Disable warnings for spaces before opening parenthesis in > hw/{ac97,adlib,cs4231a,es1370,gus,sb16}.c. > > Signed-off-by: Andreas Färber <afaerber@suse.de> > Cc: Blue Swirl <blauwirbel@gmail.com> > Cc: malc <av1474@comtv.ru> > --- > scripts/checkpatch.pl | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 8850a5f..5433736 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -246,6 +246,8 @@ our $allowed_asm_includes = qr{(?x: > )}; > # memory.h: ARM has a custom one > > +our $audio_files = qr{hw/ac97.c|hw/adlib.c|hw/cs4231a.c|hw/es1370.c|hw/gus.c|hw/sb16.c}; > + > sub build_types { > my $mods = "(?x: \n" . join("|\n ", @modifierList) . "\n)"; > my $all = "(?x: \n" . join("|\n ", @typeList) . "\n)"; > @@ -1966,6 +1968,9 @@ sub process { > asm|__asm__)$/x) > { > > + # malc wants to keep spacing consistent in the audio files. > + } elsif ($realfile =~ /($audio_files)/) { > + > # cpp #define statements have non-optional spaces, ie > # if there is a space between the name and the open > # parenthesis it is simply not a parameter group. > -- > 1.7.7 > NACK to such special casing.
On Fri, Feb 10, 2012 at 17:47, Anthony Liguori <aliguori@us.ibm.com> wrote: > On 02/09/2012 10:02 PM, malc wrote: >> >> On Fri, 10 Feb 2012, Evgeny Voevodin wrote: >> >>> On 02/09/2012 06:59 PM, Andreas F?rber wrote: >>>> >>>> Disable warnings for spaces before opening parenthesis in >>>> hw/{ac97,adlib,cs4231a,es1370,gus,sb16}.c. >>> >>> >>> Why audio files are such a special thing? >> >> >> Because they are consistently formatted the way they are. > > > I personally hate the QEMU Coding Style I dislike inconsistency more than > any particular style. I dislike unclear rules more than inconsistency or coding styles. > So I'm with malc here. I'd be opposed to introducing a new file that > deviated from Coding Style but for the ones that already do, I see no reason > to convert them all at once or make the code deviate from the style it's > already using. I'd make a rule, specify the level of importance and try to stick to it. I would not oppose global reformatting to GNU style even (which I hate) if that would be the rule. I don't like laissez faire, but if that is the rule then fine. > >> >>> Isn't it be better to revert a patch that introduced checkpatch.pl >>> errors? >> >> >> No. > > > Regards, > > Anthony Liguori > > >> >
On 02/11/2012 03:44 AM, Blue Swirl wrote: > On Fri, Feb 10, 2012 at 17:47, Anthony Liguori<aliguori@us.ibm.com> wrote: >> On 02/09/2012 10:02 PM, malc wrote: >>> >>> On Fri, 10 Feb 2012, Evgeny Voevodin wrote: >>> >>>> On 02/09/2012 06:59 PM, Andreas F?rber wrote: >>>>> >>>>> Disable warnings for spaces before opening parenthesis in >>>>> hw/{ac97,adlib,cs4231a,es1370,gus,sb16}.c. >>>> >>>> >>>> Why audio files are such a special thing? >>> >>> >>> Because they are consistently formatted the way they are. >> >> >> I personally hate the QEMU Coding Style I dislike inconsistency more than >> any particular style. > > I dislike unclear rules more than inconsistency or coding styles. > >> So I'm with malc here. I'd be opposed to introducing a new file that >> deviated from Coding Style but for the ones that already do, I see no reason >> to convert them all at once or make the code deviate from the style it's >> already using. > > I'd make a rule, specify the level of importance and try to stick to > it. I would not oppose global reformatting to GNU style even (which I > hate) if that would be the rule. I really hate having these discussions. I would almost rather we just pay the one-time cost of re-indenting so we can stop debating about this. For folks that feel strongly about this, please submit the following: An indent command that takes the tree to CODING_STYLE along with a diffstat of the end result. Depending on how bad the diffstat is, we can consider doing this and ending this set of arguments once and for all. Regards, Anthony Liguori > I don't like laissez faire, but if > that is the rule then fine. > >> >>> >>>> Isn't it be better to revert a patch that introduced checkpatch.pl >>>> errors? >>> >>> >>> No. >> >> >> Regards, >> >> Anthony Liguori >> >> >>> >> > >
Anthony Liguori <aliguori@us.ibm.com> writes: > I really hate having these discussions. I would almost rather we just > pay the one-time cost of re-indenting so we can stop debating about > this. > > For folks that feel strongly about this, please submit the following: > > An indent command that takes the tree to CODING_STYLE along with a > diffstat of the end result. > > Depending on how bad the diffstat is, we can consider doing this and > ending this set of arguments once and for all. The only justification for an idiosyncratic coding style I can buy is minimizing reindentation of old code. If we reindent anyway, reindent to something that isn't specific to the QEMU island, please.
On 02/17/2012 08:55 AM, Markus Armbruster wrote: > Anthony Liguori<aliguori@us.ibm.com> writes: > >> I really hate having these discussions. I would almost rather we just >> pay the one-time cost of re-indenting so we can stop debating about >> this. >> >> For folks that feel strongly about this, please submit the following: >> >> An indent command that takes the tree to CODING_STYLE along with a >> diffstat of the end result. >> >> Depending on how bad the diffstat is, we can consider doing this and >> ending this set of arguments once and for all. > > The only justification for an idiosyncratic coding style I can buy is > minimizing reindentation of old code. Well this was what I was getting at in my previous comments. If we just need to reindent < 10 files with a few random changes here and there, then maybe that isn't so bad. But if we have to touch every single file in the tree in a significant way, then no way is it justified. > If we reindent anyway, reindent > to something that isn't specific to the QEMU island, please. I don't even want to consider something that touches every line of code. That's effectively creating a new source tree and losing the continuity of our SCM history. Regards, Anthony Liguori
On Fri, Feb 17, 2012 at 14:31, Anthony Liguori <aliguori@us.ibm.com> wrote: > On 02/11/2012 03:44 AM, Blue Swirl wrote: >> >> On Fri, Feb 10, 2012 at 17:47, Anthony Liguori<aliguori@us.ibm.com> >> wrote: >>> >>> On 02/09/2012 10:02 PM, malc wrote: >>>> >>>> >>>> On Fri, 10 Feb 2012, Evgeny Voevodin wrote: >>>> >>>>> On 02/09/2012 06:59 PM, Andreas F?rber wrote: >>>>>> >>>>>> >>>>>> Disable warnings for spaces before opening parenthesis in >>>>>> hw/{ac97,adlib,cs4231a,es1370,gus,sb16}.c. >>>>> >>>>> >>>>> >>>>> Why audio files are such a special thing? >>>> >>>> >>>> >>>> Because they are consistently formatted the way they are. >>> >>> >>> >>> I personally hate the QEMU Coding Style I dislike inconsistency more than >>> any particular style. >> >> >> I dislike unclear rules more than inconsistency or coding styles. >> >>> So I'm with malc here. I'd be opposed to introducing a new file that >>> deviated from Coding Style but for the ones that already do, I see no >>> reason >>> to convert them all at once or make the code deviate from the style it's >>> already using. >> >> >> I'd make a rule, specify the level of importance and try to stick to >> it. I would not oppose global reformatting to GNU style even (which I >> hate) if that would be the rule. > > > I really hate having these discussions. I would almost rather we just pay > the one-time cost of re-indenting so we can stop debating about this. Fully agree on both. > For folks that feel strongly about this, please submit the following: > > An indent command that takes the tree to CODING_STYLE along with a diffstat > of the end result. > > Depending on how bad the diffstat is, we can consider doing this and ending > this set of arguments once and for all. IIRC GNU indent could not be convinced to follow QEMU style. A quick test on target-sparc with the command indent -kr -i4 -nut -sob -l80 -ss -ncs -il0 -cp1 -nfca -TCPUState -TCPUSPARCState *.[ch] is very close, but I couldn't avoid comment reformatting despite -nfca, indent is totally confused with helper.h and some casts get spaces despite -ncs. > Regards, > > Anthony Liguori > > >> I don't like laissez faire, but if >> that is the rule then fine. >> >>> >>>> >>>>> Isn't it be better to revert a patch that introduced checkpatch.pl >>>>> errors? >>>> >>>> >>>> >>>> No. >>> >>> >>> >>> Regards, >>> >>> Anthony Liguori >>> >>> >>>> >>> >> >> >
On Fri, Feb 17, 2012 at 15:26, Anthony Liguori <aliguori@us.ibm.com> wrote: > On 02/17/2012 08:55 AM, Markus Armbruster wrote: >> >> Anthony Liguori<aliguori@us.ibm.com> writes: >> >>> I really hate having these discussions. I would almost rather we just >>> pay the one-time cost of re-indenting so we can stop debating about >>> this. >>> >>> For folks that feel strongly about this, please submit the following: >>> >>> An indent command that takes the tree to CODING_STYLE along with a >>> diffstat of the end result. >>> >>> Depending on how bad the diffstat is, we can consider doing this and >>> ending this set of arguments once and for all. >> >> >> The only justification for an idiosyncratic coding style I can buy is >> minimizing reindentation of old code. > > > Well this was what I was getting at in my previous comments. If we just > need to reindent < 10 files with a few random changes here and there, then > maybe that isn't so bad. > > But if we have to touch every single file in the tree in a significant way, > then no way is it justified. One way to handle this is gradual reformatting, every time when code is touched, only changes towards common CODING_STYLE are allowed. Small, contained reformatting patches should be also allowed, for example to adjust brace style in one file a time or to remove spaces at the end of line. >> If we reindent anyway, reindent >> to something that isn't specific to the QEMU island, please. > > > I don't even want to consider something that touches every line of code. > That's effectively creating a new source tree and losing the continuity of > our SCM history. I think only 'git blame' output would be affected and that is not 100% reliable anyway, considering for example code movement. > Regards, > > Anthony Liguori > >
Am 18.02.2012 10:13, schrieb Blue Swirl: > On Fri, Feb 17, 2012 at 15:26, Anthony Liguori <aliguori@us.ibm.com> > wrote: >> Well this was what I was getting at in my previous comments. If we just >> need to reindent < 10 files with a few random changes here and there, >> then >> maybe that isn't so bad. >> >> But if we have to touch every single file in the tree in a >> significant way, >> then no way is it justified. > > One way to handle this is gradual reformatting, every time when code > is touched, only changes towards common CODING_STYLE are allowed. > Small, contained reformatting patches should be also allowed, for > example to adjust brace style in one file a time or to remove spaces > at the end of line. I'd appreciate it very much if we could remove spaces at line endings for all non binary files as soon as possible. Newer versions of git (maybe also older ones with appropriate configuration, see 'git config --help', core.whitespace / blank-at-eol/ apply.whitespace) complain about patches which add such spaces. Nevertheless even recent patches did add spaces, so obviously not all committers use that git settings. It's good practice to use an editor which automatically removes spaces at end of line (I think most editors can be configured to do this). With the current code, this is difficult because it introduces additional whitespace changes when I edit a file with spaces which are removed by the editor. > >> >> I don't even want to consider something that touches every line of code. >> That's effectively creating a new source tree and losing the >> continuity of >> our SCM history. > > I think only 'git blame' output would be affected and that is not 100% > reliable anyway, considering for example code movement. 'git blame' can optionally ignore whitespace changes, so that is not really a problem. It can even ignore code movements :-) Just use 'git blame -w -C'. Regards, Stefan
On 02/18/2012 04:56 AM, Stefan Weil wrote: > Am 18.02.2012 10:13, schrieb Blue Swirl: >> On Fri, Feb 17, 2012 at 15:26, Anthony Liguori <aliguori@us.ibm.com> wrote: >>> Well this was what I was getting at in my previous comments. If we just >>> need to reindent < 10 files with a few random changes here and there, then >>> maybe that isn't so bad. >>> >>> But if we have to touch every single file in the tree in a significant way, >>> then no way is it justified. >> >> One way to handle this is gradual reformatting, every time when code >> is touched, only changes towards common CODING_STYLE are allowed. >> Small, contained reformatting patches should be also allowed, for >> example to adjust brace style in one file a time or to remove spaces >> at the end of line. > > I'd appreciate it very much if we could remove spaces at line endings > for all non binary files as soon as possible. Why? I really can't understand this. It's not visible so it does no harm. Regards, Anthony Liguori
On Sat, Feb 18, 2012 at 14:18, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 02/18/2012 04:56 AM, Stefan Weil wrote: >> >> Am 18.02.2012 10:13, schrieb Blue Swirl: >>> >>> On Fri, Feb 17, 2012 at 15:26, Anthony Liguori <aliguori@us.ibm.com> >>> wrote: >>>> >>>> Well this was what I was getting at in my previous comments. If we just >>>> need to reindent < 10 files with a few random changes here and there, >>>> then >>>> maybe that isn't so bad. >>>> >>>> But if we have to touch every single file in the tree in a significant >>>> way, >>>> then no way is it justified. >>> >>> >>> One way to handle this is gradual reformatting, every time when code >>> is touched, only changes towards common CODING_STYLE are allowed. >>> Small, contained reformatting patches should be also allowed, for >>> example to adjust brace style in one file a time or to remove spaces >>> at the end of line. >> >> >> I'd appreciate it very much if we could remove spaces at line endings >> for all non binary files as soon as possible. > > > Why? > > I really can't understand this. It's not visible so it does no harm. It's not the end of the world, but they could be mangled (which will make patches fail to apply) when mailed and they waste precious bytes. They are also not useful in any way, so better remove them. > Regards, > > Anthony Liguori
Am 18.02.2012 10:13, schrieb Blue Swirl: > One way to handle this is gradual reformatting, every time when code > is touched, only changes towards common CODING_STYLE are allowed. That's what we've been doing wrt braces and I appreciate us not cluttering the history with reformatting commits. Especially at a time where I'm touching every target (and seeing quite some variations on coding style) I'm not so happy about this initiative. > On Fri, Feb 17, 2012 at 15:26, Anthony Liguori <aliguori@us.ibm.com> wrote: >> I don't even want to consider something that touches every line of code. >> That's effectively creating a new source tree and losing the continuity of >> our SCM history. > > I think only 'git blame' output would be affected and that is not 100% > reliable anyway, considering for example code movement. I use repo.or.cz's blame function quite often to find out who to cc or what commit to mention, and (valid) typo fixes are already troublesome, requiring to go to the parent commit and to navigate to the same file in its tree. One cannot specify any custom ignore options there. Is there a better tool to do such interactive, recursive git-blame? Andreas
On 02/18/2012 08:53 AM, Andreas Färber wrote: >> I think only 'git blame' output would be affected and that is not 100% >> reliable anyway, considering for example code movement. > > I use repo.or.cz's blame function quite often to find out who to cc or > what commit to mention, and (valid) typo fixes are already troublesome, > requiring to go to the parent commit and to navigate to the same file in > its tree. One cannot specify any custom ignore options there. > > Is there a better tool to do such interactive, recursive git-blame? I use 'git gui blame $commit $file &' for interactive recursive blaming. When you find the line that you are interested in blaming, you can click on the commit link on the left side to see the context at the time of the commit that introduced the state of that line, then right click on the right side (the line in question) to easily trigger a 'blame parent' operation. It's about the only thing that I use 'git gui' for, but I find it well worth the pain of using a tcl frontend.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 8850a5f..5433736 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -246,6 +246,8 @@ our $allowed_asm_includes = qr{(?x: )}; # memory.h: ARM has a custom one +our $audio_files = qr{hw/ac97.c|hw/adlib.c|hw/cs4231a.c|hw/es1370.c|hw/gus.c|hw/sb16.c}; + sub build_types { my $mods = "(?x: \n" . join("|\n ", @modifierList) . "\n)"; my $all = "(?x: \n" . join("|\n ", @typeList) . "\n)"; @@ -1966,6 +1968,9 @@ sub process { asm|__asm__)$/x) { + # malc wants to keep spacing consistent in the audio files. + } elsif ($realfile =~ /($audio_files)/) { + # cpp #define statements have non-optional spaces, ie # if there is a space between the name and the open # parenthesis it is simply not a parameter group.
Disable warnings for spaces before opening parenthesis in hw/{ac97,adlib,cs4231a,es1370,gus,sb16}.c. Signed-off-by: Andreas Färber <afaerber@suse.de> Cc: Blue Swirl <blauwirbel@gmail.com> Cc: malc <av1474@comtv.ru> --- scripts/checkpatch.pl | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)