| Message ID | 1280593414-2232-1-git-send-email-av1474@comtv.ru |
|---|---|
| State | New |
| Headers | show |
On Sat, Jul 31, 2010 at 08:23:34PM +0400, malc wrote: > History has shown that this particular rule is unenforcable. > > Signed-off-by: malc <av1474@comtv.ru> > --- > CODING_STYLE | 11 ++++++----- > 1 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/CODING_STYLE b/CODING_STYLE > index 92036f3..e0b5376 100644 > --- a/CODING_STYLE > +++ b/CODING_STYLE > @@ -54,16 +54,17 @@ readers that they are seeing a wrapped version; otherwise avoid this prefix. > > 4. Block structure > > -Every indented statement is braced; even if the block contains just one > -statement. The opening brace is on the line that contains the control > -flow statement that introduces the new block; the closing brace is on the > -same line as the else keyword, or on a line by itself if there is no else > -keyword. Example: > +The opening brace is on the line that contains the control flow > +statement that introduces the new block; the closing brace is on the > +same line as the else keyword, or on a line by itself if there is no > +else keyword. Example: > > if (a == 5) { > printf("a was 5.\n"); > + do5stuff(); > } else if (a == 6) { > printf("a was 6.\n"); > + do6stuff(); > } else { > printf("a was something else entirely.\n"); > } I am "neutral" on this particular rule. OTOH, my opinion is that we really should try to enforce the rules. The fact that we are humans and make mistakes (some patches not enforcing all the rules are committed), is not enough to just ignore the rules. If a rule is not enforceable, it should be removed from CODING_STYLE, not removed.
On Sat, Jul 31, 2010 at 06:47:29PM +0200, Aurelien Jarno wrote: > On Sat, Jul 31, 2010 at 08:23:34PM +0400, malc wrote: > > History has shown that this particular rule is unenforcable. > > > > Signed-off-by: malc <av1474@comtv.ru> > > --- > > CODING_STYLE | 11 ++++++----- > > 1 files changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/CODING_STYLE b/CODING_STYLE > > index 92036f3..e0b5376 100644 > > --- a/CODING_STYLE > > +++ b/CODING_STYLE > > @@ -54,16 +54,17 @@ readers that they are seeing a wrapped version; otherwise avoid this prefix. > > > > 4. Block structure > > > > -Every indented statement is braced; even if the block contains just one > > -statement. The opening brace is on the line that contains the control > > -flow statement that introduces the new block; the closing brace is on the > > -same line as the else keyword, or on a line by itself if there is no else > > -keyword. Example: > > +The opening brace is on the line that contains the control flow > > +statement that introduces the new block; the closing brace is on the > > +same line as the else keyword, or on a line by itself if there is no > > +else keyword. Example: > > > > if (a == 5) { > > printf("a was 5.\n"); > > + do5stuff(); > > } else if (a == 6) { > > printf("a was 6.\n"); > > + do6stuff(); > > } else { > > printf("a was something else entirely.\n"); > > } > > I am "neutral" on this particular rule. > > OTOH, my opinion is that we really should try to enforce the rules. The > fact that we are humans and make mistakes (some patches not enforcing > all the rules are committed), is not enough to just ignore the rules. If > a rule is not enforceable, it should be removed from CODING_STYLE, not > removed. ^^^^^^^ Oops, you should read "ignored" here.
On Sat, Jul 31, 2010 at 4:23 PM, malc <av1474@comtv.ru> wrote: > History has shown that this particular rule is unenforcable. > > Signed-off-by: malc <av1474@comtv.ru> > --- > CODING_STYLE | 11 ++++++----- > 1 files changed, 6 insertions(+), 5 deletions(-) Not again: http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg00484.html There are plenty of ways to make the rule enforceable, for example we could agree to start to revert commits which introduce new CODING_STYLE violations.
On Sat, 31 Jul 2010, Blue Swirl wrote: > On Sat, Jul 31, 2010 at 4:23 PM, malc <av1474@comtv.ru> wrote: > > History has shown that this particular rule is unenforcable. > > > > Signed-off-by: malc <av1474@comtv.ru> > > --- > > CODING_STYLE | 11 ++++++----- > > 1 files changed, 6 insertions(+), 5 deletions(-) > > Not again: > http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg00484.html I'm not yet senile enough to forget that, let me quote some: "By picking a single coding style we've pretty much guaranteed that most people will disagree with some of it. IMO consistency is more important." We don't have consistency, people, myself included (which was a suprise), keep applying patches that break things in this particular regard, it should just go. > > There are plenty of ways to make the rule enforceable, for example we > could agree to start to revert commits which introduce new > CODING_STYLE violations. > There might be plenty of ways, but none were/are used, we somehow managed to enforce tab rule, but the braces remain elusive.
On Sat, Jul 31, 2010 at 08:23:55PM +0000, Blue Swirl wrote: > On Sat, Jul 31, 2010 at 4:23 PM, malc <av1474@comtv.ru> wrote: > > History has shown that this particular rule is unenforcable. > > > > Signed-off-by: malc <av1474@comtv.ru> > > --- > > CODING_STYLE | 11 ++++++----- > > 1 files changed, 6 insertions(+), 5 deletions(-) > > Not again: > http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg00484.html > > There are plenty of ways to make the rule enforceable, for example we > could agree to start to revert commits which introduce new > CODING_STYLE violations. > It seems to be possible to add a pre-applypatch script to the git hook directory, that will verify the commit and reject it if it doesn't comply with the coding rules. Of course it's possible to commit a patch anyway by using --no-verify. The good point of this approach is that the rule is enforced by a script, which is not suppose to make mistakes, and that it can be shared between patch submitters and patch committers: both side can make mistakes and it is always better to know that as early as possible. Of course someone as to translate the coding rules in a set of regular expressions able to catch errors.
On 07/31/2010 06:49 PM, Aurelien Jarno wrote: > On Sat, Jul 31, 2010 at 08:23:55PM +0000, Blue Swirl wrote: > >> On Sat, Jul 31, 2010 at 4:23 PM, malc<av1474@comtv.ru> wrote: >> >>> History has shown that this particular rule is unenforcable. >>> >>> Signed-off-by: malc<av1474@comtv.ru> >>> --- >>> CODING_STYLE | 11 ++++++----- >>> 1 files changed, 6 insertions(+), 5 deletions(-) >>> >> Not again: >> http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg00484.html >> >> There are plenty of ways to make the rule enforceable, for example we >> could agree to start to revert commits which introduce new >> CODING_STYLE violations. >> >> > It seems to be possible to add a pre-applypatch script to the git hook > directory, that will verify the commit and reject it if it doesn't > comply with the coding rules. Of course it's possible to commit a patch > anyway by using --no-verify. > There are certain aspects of CODING_STYLE that I think are pretty important. For instance, space vs. tabs can really screw things up for people that have non-standard tabs. This is something that enforcing at patch submission time seems to be really important. Type naming seems important too because it's often not isolated. IOW, a poor choice in one file can end up polluting other files quickly that require interacting. The result is a mess of naming styles. But something like braces around an if doesn't seem like it creates a big problem. Most C programmers are used to seeing braces in some statements and not other. Therefore, it's hard to argue that the code gets really unreadable if this isn't strictly enforced. So really, I think the problem is that we're enforcing the words of CODING_STYLE instead of the intent. The intent of CODING_STYLE is to improve the readability of the code. I think it requires a certain amount of taste to be applied. Rejecting a big patch because braces aren't used in single line if statements seems to be an unnecessary barrier to me. Regards, Anthony Liguori > The good point of this approach is that the rule is enforced by a > script, which is not suppose to make mistakes, and that it can be shared > between patch submitters and patch committers: both side can make > mistakes and it is always better to know that as early as possible. > > Of course someone as to translate the coding rules in a set of regular > expressions able to catch errors. > >
Am 02.08.2010 17:20, schrieb Anthony Liguori: > On 07/31/2010 06:49 PM, Aurelien Jarno wrote: >> On Sat, Jul 31, 2010 at 08:23:55PM +0000, Blue Swirl wrote: >> >>> On Sat, Jul 31, 2010 at 4:23 PM, malc<av1474@comtv.ru> wrote: >>> >>>> History has shown that this particular rule is unenforcable. >>>> >>>> Signed-off-by: malc<av1474@comtv.ru> >>>> --- >>>> CODING_STYLE | 11 ++++++----- >>>> 1 files changed, 6 insertions(+), 5 deletions(-) >>>> >>> Not again: >>> http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg00484.html >>> >>> There are plenty of ways to make the rule enforceable, for example we >>> could agree to start to revert commits which introduce new >>> CODING_STYLE violations. >>> >>> >> It seems to be possible to add a pre-applypatch script to the git hook >> directory, that will verify the commit and reject it if it doesn't >> comply with the coding rules. Of course it's possible to commit a patch >> anyway by using --no-verify. >> > > There are certain aspects of CODING_STYLE that I think are pretty > important. For instance, space vs. tabs can really screw things up for > people that have non-standard tabs. This is something that enforcing at > patch submission time seems to be really important. > > Type naming seems important too because it's often not isolated. IOW, a > poor choice in one file can end up polluting other files quickly that > require interacting. The result is a mess of naming styles. > > But something like braces around an if doesn't seem like it creates a > big problem. Most C programmers are used to seeing braces in some > statements and not other. Therefore, it's hard to argue that the code > gets really unreadable if this isn't strictly enforced. I won't argue that missing braces impact readability of the code, they probably don't. However, as was pointed out in earlier discussion there still remain two important points: 1. While it doesn't make a difference for the code itself, readability of patches suffers when braces have to be added/removed when a second line is inserted or disappears. 2. I've messed up more than once with adding some debug code (even worse when it happens with real code): if (foo) fprintf(stderr, "debug msg\n"); bar(); /* oops, not conditional any more */ This is why I tend to disagree with removing the rule, and suggest to rather implement some automatic checks like Aurelien suggested (if we need to change anything at all). I usually don't ask for a respin just for braces if the patch is good otherwise, but if you think we should just reject such patches without exceptions, I can change that. > So really, I think the problem is that we're enforcing the words of > CODING_STYLE instead of the intent. The intent of CODING_STYLE is to > improve the readability of the code. I think it requires a certain > amount of taste to be applied. > > Rejecting a big patch because braces aren't used in single line if > statements seems to be an unnecessary barrier to me. Taking such patches anyway is basically what we're doing today, right? And what malc is complaining about. Kevin
On 08/02/2010 10:41 AM, Kevin Wolf wrote: > >> But something like braces around an if doesn't seem like it creates a >> big problem. Most C programmers are used to seeing braces in some >> statements and not other. Therefore, it's hard to argue that the code >> gets really unreadable if this isn't strictly enforced. >> > I won't argue that missing braces impact readability of the code, they > probably don't. However, as was pointed out in earlier discussion there > still remain two important points: > > 1. While it doesn't make a difference for the code itself, readability > of patches suffers when braces have to be added/removed when a second > line is inserted or disappears. > I understand the argument but it's not something that I strongly agree with. > 2. I've messed up more than once with adding some debug code (even worse > when it happens with real code): > > if (foo) > fprintf(stderr, "debug msg\n"); > bar(); /* oops, not conditional any more */ > This is hard to do with editors that auto indent unless you're copying and pasting debugging. And yeah, I've made that mistake too doing the later :-) > This is why I tend to disagree with removing the rule, and suggest to > rather implement some automatic checks like Aurelien suggested (if we > need to change anything at all). I usually don't ask for a respin just > for braces if the patch is good otherwise, but if you think we should > just reject such patches without exceptions, I can change that. > Yeah, I try to remember to enforce it but often forget or just don't notice. My eyes don't tend to catch missing braces like they would catch bad type naming because the former is really not uncommon. I'm happy with the status quo but I won't object to a git commit hook that enforces style. Regards, Anthony Liguori
On Mon, 2 Aug 2010, Kevin Wolf wrote: > Am 02.08.2010 17:20, schrieb Anthony Liguori: > > On 07/31/2010 06:49 PM, Aurelien Jarno wrote: > >> On Sat, Jul 31, 2010 at 08:23:55PM +0000, Blue Swirl wrote: [..snip..] > > > So really, I think the problem is that we're enforcing the words of > > CODING_STYLE instead of the intent. The intent of CODING_STYLE is to > > improve the readability of the code. I think it requires a certain > > amount of taste to be applied. > > > Rejecting a big patch because braces aren't used in single line if > > statements seems to be an unnecessary barrier to me. > > Taking such patches anyway is basically what we're doing today, right? > And what malc is complaining about. Malc is mainly complaining about "Quod licet Jovi non licet bovi" situation.
On 08/02/2010 10:55 AM, malc wrote: >>> Rejecting a big patch because braces aren't used in single line if >>> statements seems to be an unnecessary barrier to me. >>> >> Taking such patches anyway is basically what we're doing today, right? >> And what malc is complaining about. >> > Malc is mainly complaining about "Quod licet Jovi non licet bovi" > situation. > Very fair point. Regards, Anthony Liguori
On Mon, 2 Aug 2010, Anthony Liguori wrote: > On 08/02/2010 10:41 AM, Kevin Wolf wrote: > > > > > But something like braces around an if doesn't seem like it creates a > > > big problem. Most C programmers are used to seeing braces in some > > > statements and not other. Therefore, it's hard to argue that the code > > > gets really unreadable if this isn't strictly enforced. > > > > > I won't argue that missing braces impact readability of the code, they > > probably don't. However, as was pointed out in earlier discussion there > > still remain two important points: > > > > 1. While it doesn't make a difference for the code itself, readability > > of patches suffers when braces have to be added/removed when a second > > line is inserted or disappears. > > > > I understand the argument but it's not something that I strongly agree with. > > > 2. I've messed up more than once with adding some debug code (even worse > > when it happens with real code): > > > > if (foo) > > fprintf(stderr, "debug msg\n"); > > bar(); /* oops, not conditional any more */ > > > > This is hard to do with editors that auto indent unless you're copying and > pasting debugging. And yeah, I've made that mistake too doing the later :-) > > > This is why I tend to disagree with removing the rule, and suggest to > > rather implement some automatic checks like Aurelien suggested (if we > > need to change anything at all). I usually don't ask for a respin just > > for braces if the patch is good otherwise, but if you think we should > > just reject such patches without exceptions, I can change that. > > > > Yeah, I try to remember to enforce it but often forget or just don't notice. > My eyes don't tend to catch missing braces like they would catch bad type > naming because the former is really not uncommon. > > I'm happy with the status quo but I won't object to a git commit hook that > enforces style. Seriously? You are happy with the situation where some people get their patches rejected because they disagree/forogot/don't_care about single statement braces while the patches of others make it through?
On 08/02/2010 11:06 AM, malc wrote: > On Mon, 2 Aug 2010, Anthony Liguori wrote: > > >> On 08/02/2010 10:41 AM, Kevin Wolf wrote: >> >>> >>>> But something like braces around an if doesn't seem like it creates a >>>> big problem. Most C programmers are used to seeing braces in some >>>> statements and not other. Therefore, it's hard to argue that the code >>>> gets really unreadable if this isn't strictly enforced. >>>> >>>> >>> I won't argue that missing braces impact readability of the code, they >>> probably don't. However, as was pointed out in earlier discussion there >>> still remain two important points: >>> >>> 1. While it doesn't make a difference for the code itself, readability >>> of patches suffers when braces have to be added/removed when a second >>> line is inserted or disappears. >>> >>> >> I understand the argument but it's not something that I strongly agree with. >> >> >>> 2. I've messed up more than once with adding some debug code (even worse >>> when it happens with real code): >>> >>> if (foo) >>> fprintf(stderr, "debug msg\n"); >>> bar(); /* oops, not conditional any more */ >>> >>> >> This is hard to do with editors that auto indent unless you're copying and >> pasting debugging. And yeah, I've made that mistake too doing the later :-) >> >> >>> This is why I tend to disagree with removing the rule, and suggest to >>> rather implement some automatic checks like Aurelien suggested (if we >>> need to change anything at all). I usually don't ask for a respin just >>> for braces if the patch is good otherwise, but if you think we should >>> just reject such patches without exceptions, I can change that. >>> >>> >> Yeah, I try to remember to enforce it but often forget or just don't notice. >> My eyes don't tend to catch missing braces like they would catch bad type >> naming because the former is really not uncommon. >> >> I'm happy with the status quo but I won't object to a git commit hook that >> enforces style. >> > Seriously? You are happy with the situation where some people get their > patches rejected because they disagree/forogot/don't_care about single > statement braces while the patches of others make it through? > Yeah, I'm neglecting the fact that we're not consistent as maintainers and I'm all for dropping it from CODING_STYLE. Regards, Anthony Liguori
On Mon, Aug 2, 2010 at 3:20 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 07/31/2010 06:49 PM, Aurelien Jarno wrote: >> >> On Sat, Jul 31, 2010 at 08:23:55PM +0000, Blue Swirl wrote: >> >>> >>> On Sat, Jul 31, 2010 at 4:23 PM, malc<av1474@comtv.ru> wrote: >>> >>>> >>>> History has shown that this particular rule is unenforcable. >>>> >>>> Signed-off-by: malc<av1474@comtv.ru> >>>> --- >>>> CODING_STYLE | 11 ++++++----- >>>> 1 files changed, 6 insertions(+), 5 deletions(-) >>>> >>> >>> Not again: >>> http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg00484.html >>> >>> There are plenty of ways to make the rule enforceable, for example we >>> could agree to start to revert commits which introduce new >>> CODING_STYLE violations. >>> >>> >> >> It seems to be possible to add a pre-applypatch script to the git hook >> directory, that will verify the commit and reject it if it doesn't >> comply with the coding rules. Of course it's possible to commit a patch >> anyway by using --no-verify. >> > > There are certain aspects of CODING_STYLE that I think are pretty important. > For instance, space vs. tabs can really screw things up for people that > have non-standard tabs. This is something that enforcing at patch > submission time seems to be really important. > > Type naming seems important too because it's often not isolated. IOW, a > poor choice in one file can end up polluting other files quickly that > require interacting. The result is a mess of naming styles. > > But something like braces around an if doesn't seem like it creates a big > problem. Most C programmers are used to seeing braces in some statements > and not other. Therefore, it's hard to argue that the code gets really > unreadable if this isn't strictly enforced. > > So really, I think the problem is that we're enforcing the words of > CODING_STYLE instead of the intent. The intent of CODING_STYLE is to > improve the readability of the code. I think it requires a certain amount > of taste to be applied. The problem with that approach is that CODING_STYLE is not written like that at all. All four requirements equally state that they describe QEMU coding style. There is also no room left for interpretation: "Every indented statement is braced; even if the block contains just one statement."
On Mon, Aug 2, 2010 at 4:18 PM, Anthony Liguori <anthony@codemonkey.ws> wrote: > On 08/02/2010 11:06 AM, malc wrote: >> >> On Mon, 2 Aug 2010, Anthony Liguori wrote: >> >> >>> >>> On 08/02/2010 10:41 AM, Kevin Wolf wrote: >>> >>>> >>>> >>>>> >>>>> But something like braces around an if doesn't seem like it creates a >>>>> big problem. Most C programmers are used to seeing braces in some >>>>> statements and not other. Therefore, it's hard to argue that the code >>>>> gets really unreadable if this isn't strictly enforced. >>>>> >>>>> >>>> >>>> I won't argue that missing braces impact readability of the code, they >>>> probably don't. However, as was pointed out in earlier discussion there >>>> still remain two important points: >>>> >>>> 1. While it doesn't make a difference for the code itself, readability >>>> of patches suffers when braces have to be added/removed when a second >>>> line is inserted or disappears. >>>> >>>> >>> >>> I understand the argument but it's not something that I strongly agree >>> with. >>> >>> >>>> >>>> 2. I've messed up more than once with adding some debug code (even worse >>>> when it happens with real code): >>>> >>>> if (foo) >>>> fprintf(stderr, "debug msg\n"); >>>> bar(); /* oops, not conditional any more */ >>>> >>>> >>> >>> This is hard to do with editors that auto indent unless you're copying >>> and >>> pasting debugging. And yeah, I've made that mistake too doing the later >>> :-) >>> >>> >>>> >>>> This is why I tend to disagree with removing the rule, and suggest to >>>> rather implement some automatic checks like Aurelien suggested (if we >>>> need to change anything at all). I usually don't ask for a respin just >>>> for braces if the patch is good otherwise, but if you think we should >>>> just reject such patches without exceptions, I can change that. >>>> >>>> >>> >>> Yeah, I try to remember to enforce it but often forget or just don't >>> notice. >>> My eyes don't tend to catch missing braces like they would catch bad type >>> naming because the former is really not uncommon. >>> >>> I'm happy with the status quo but I won't object to a git commit hook >>> that >>> enforces style. >>> >> >> Seriously? You are happy with the situation where some people get their >> patches rejected because they disagree/forogot/don't_care about single >> statement braces while the patches of others make it through? >> > > Yeah, I'm neglecting the fact that we're not consistent as maintainers and > I'm all for dropping it from CODING_STYLE. I'd rather expand the document. For example, I like the approach libvirt takes: http://libvirt.org/git/?p=libvirt.git;a=blob_plain;f=HACKING;hb=HEAD Not specifically to braces, but they describe types and memory allocation etc. which is FAQ stuff for us too.
On 08/02/2010 11:29 AM, Blue Swirl wrote: >> Yeah, I'm neglecting the fact that we're not consistent as maintainers and >> I'm all for dropping it from CODING_STYLE. >> > I'd rather expand the document. For example, I like the approach libvirt takes: > http://libvirt.org/git/?p=libvirt.git;a=blob_plain;f=HACKING;hb=HEAD > > Not specifically to braces, but they describe types and memory > allocation etc. which is FAQ stuff for us too. > I agree 100%. There are a lot of idioms in QEMU that would be good to document for people learning the code base. But I think that's orthogonal to the discussion of whether we want to keep the if rule and whether we want to enforce all of coding style at commit time. Regards, Anthony Liguori
diff --git a/CODING_STYLE b/CODING_STYLE index 92036f3..e0b5376 100644 --- a/CODING_STYLE +++ b/CODING_STYLE @@ -54,16 +54,17 @@ readers that they are seeing a wrapped version; otherwise avoid this prefix. 4. Block structure -Every indented statement is braced; even if the block contains just one -statement. The opening brace is on the line that contains the control -flow statement that introduces the new block; the closing brace is on the -same line as the else keyword, or on a line by itself if there is no else -keyword. Example: +The opening brace is on the line that contains the control flow +statement that introduces the new block; the closing brace is on the +same line as the else keyword, or on a line by itself if there is no +else keyword. Example: if (a == 5) { printf("a was 5.\n"); + do5stuff(); } else if (a == 6) { printf("a was 6.\n"); + do6stuff(); } else { printf("a was something else entirely.\n"); }
History has shown that this particular rule is unenforcable. Signed-off-by: malc <av1474@comtv.ru> --- CODING_STYLE | 11 ++++++----- 1 files changed, 6 insertions(+), 5 deletions(-)