| Message ID | 1516761089-90086-1-git-send-email-jpettit@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] checkpatch.py: Add check for "xxx" in comments. | expand |
> -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Justin Pettit > Sent: Wednesday, January 24, 2018 2:31 AM > To: dev@openvswitch.org > Subject: [ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in comments. > > "xxx" is often used to indicate items that the developer wanted to look at > again before committing. Flag those as a warning. > Hi Justin, Does this mean that code that contains 'xxx' should not be accepted? I guess ideally we'd want a clean run from the checkpatch script when submitting/reviewing patches. I'm just thinking of cases where there is workaround in place in the code marked with a 'xxx' but with the intention that it will be replaced in the future (sometimes we see this with DPDK where and upcoming DPDK release will provide a better solution). Should a user flag future re-work with a separate tag in the comments? I know in the coding style docs we suggest using XXX or FIXME comments to mark code that needs work. Thanks Ian > Signed-off-by: Justin Pettit <jpettit@ovn.org> > --- > utilities/checkpatch.py | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py index > 33feb6bddca1..42777e6fcb15 100755 > --- a/utilities/checkpatch.py > +++ b/utilities/checkpatch.py > @@ -1,5 +1,6 @@ > #!/usr/bin/env python > # Copyright (c) 2016, 2017 Red Hat, Inc. > +# Copyright (c) 2018 Nicira, Inc. > # > # Licensed under the Apache License, Version 2.0 (the "License"); # you > may not use this file except in compliance with the License. > @@ -95,9 +96,11 @@ __regex_ends_with_bracket = \ > re.compile(r'[^\s]\) {(\s+/\*[\s\Sa-zA-Z0-9\.,\?\*/+-]*)?$') > __regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0- > 9]\*[^*]') __regex_is_comment_line = re.compile(r'^\s*(/\*|\*\s)') > +__regex_has_comment = re.compile(r'.*(/\*|\*\s)') > __regex_trailing_operator = re.compile(r'^[^ ]* [^ ]*[?:]$') > __regex_conditional_else_bracing = re.compile(r'^\s*else\s*{?$') > __regex_conditional_else_bracing2 = re.compile(r'^\s*}\selse\s*$') > +__regex_has_xxx_mark = re.compile(r'.*xxx.*', re.IGNORECASE) > > skip_leading_whitespace_check = False > skip_trailing_whitespace_check = False > @@ -213,11 +216,19 @@ def is_comment_line(line): > """Returns TRUE if the current line is part of a block comment.""" > return __regex_is_comment_line.match(line) is not None > > +def has_comment(line): > + """Returns TRUE if the current line contains a comment or is part of > + a block comment.""" > + return __regex_has_comment.match(line) is not None > > def trailing_operator(line): > """Returns TRUE if the current line ends with an operatorsuch as ? or > :""" > return __regex_trailing_operator.match(line) is not None > > +def has_xxx_mark(line): > + """Returns TRUE if the current line contains 'xxx'.""" > + return __regex_has_xxx_mark.match(line) is not None > + > > checks = [ > {'regex': None, > @@ -257,6 +268,11 @@ checks = [ > 'check': lambda x: trailing_operator(x), > 'print': > lambda: print_error("Line has '?' or ':' operator at end of line")}, > + > + {'regex': '(\.c|\.h)(\.in)?$', 'match_name': None, > + 'prereq': lambda x: has_comment(x), > + 'check': lambda x: has_xxx_mark(x), > + 'print': lambda: print_warning("Comment with 'xxx' marker")}, > ] > > > -- > 2.7.4 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Jan 24, 2018 at 01:55:18PM +0000, Stokes, Ian wrote: > > -----Original Message----- > > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > > bounces@openvswitch.org] On Behalf Of Justin Pettit > > Sent: Wednesday, January 24, 2018 2:31 AM > > To: dev@openvswitch.org > > Subject: [ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in comments. > > > > "xxx" is often used to indicate items that the developer wanted to look at > > again before committing. Flag those as a warning. > > Does this mean that code that contains 'xxx' should not be accepted? I guess ideally we'd want a clean run from the checkpatch script when submitting/reviewing patches. I guess that clean "checkpatch" is ideal, but I apply a lot of patches that do give checkpatch warnings because checkpatch isn't perfect. I think of checkpatch as something that raises possible issues that a human should look at.
> On Jan 24, 2018, at 9:14 AM, Ben Pfaff <blp@ovn.org> wrote: > > On Wed, Jan 24, 2018 at 01:55:18PM +0000, Stokes, Ian wrote: >>> -----Original Message----- >>> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- >>> bounces@openvswitch.org] On Behalf Of Justin Pettit >>> Sent: Wednesday, January 24, 2018 2:31 AM >>> To: dev@openvswitch.org >>> Subject: [ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in comments. >>> >>> "xxx" is often used to indicate items that the developer wanted to look at >>> again before committing. Flag those as a warning. >> >> Does this mean that code that contains 'xxx' should not be accepted? I guess ideally we'd want a clean run from the checkpatch script when submitting/reviewing patches. > > I guess that clean "checkpatch" is ideal, but I apply a lot of patches > that do give checkpatch warnings because checkpatch isn't perfect. I > think of checkpatch as something that raises possible issues that a > human should look at. Yes, this was my thinking too. I don't think we should have a prohibition against using "xxx", but I do usually see it in patches as something that was left behind unintentionally. That said, my personal preference would be to not have that often in the codebase, since it usually indicates something half-baked, and more often than not, seems to be something no one comes back to address later on. --Justin
> > On Jan 24, 2018, at 9:14 AM, Ben Pfaff <blp@ovn.org> wrote: > > > > On Wed, Jan 24, 2018 at 01:55:18PM +0000, Stokes, Ian wrote: > >>> -----Original Message----- > >>> From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > >>> bounces@openvswitch.org] On Behalf Of Justin Pettit > >>> Sent: Wednesday, January 24, 2018 2:31 AM > >>> To: dev@openvswitch.org > >>> Subject: [ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in > comments. > >>> > >>> "xxx" is often used to indicate items that the developer wanted to > >>> look at again before committing. Flag those as a warning. > >> > >> Does this mean that code that contains 'xxx' should not be accepted? I > guess ideally we'd want a clean run from the checkpatch script when > submitting/reviewing patches. > > > > I guess that clean "checkpatch" is ideal, but I apply a lot of patches > > that do give checkpatch warnings because checkpatch isn't perfect. I > > think of checkpatch as something that raises possible issues that a > > human should look at. > > Yes, this was my thinking too. I don't think we should have a prohibition > against using "xxx", but I do usually see it in patches as something that > was left behind unintentionally. That said, my personal preference would > be to not have that often in the codebase, since it usually indicates > something half-baked, and more often than not, seems to be something no > one comes back to address later on. I'd agree, I guess there should be a valid explanation as to why it hasn't been implemented in the associated XXX comment. It'll definitely help to raise it as a point of discussion during the review. Sounds good to me. Ian > > --Justin >
> On Jan 25, 2018, at 10:26 AM, Stokes, Ian <ian.stokes@intel.com> wrote: > >> Yes, this was my thinking too. I don't think we should have a prohibition >> against using "xxx", but I do usually see it in patches as something that >> was left behind unintentionally. That said, my personal preference would >> be to not have that often in the codebase, since it usually indicates >> something half-baked, and more often than not, seems to be something no >> one comes back to address later on. > > I'd agree, I guess there should be a valid explanation as to why it hasn't been implemented in the associated XXX comment. It'll definitely help to raise it as a point of discussion during the review. Sounds good to me. Great. Thanks! --Justin
Justin Pettit <jpettit@ovn.org> writes: >> On Jan 25, 2018, at 10:26 AM, Stokes, Ian <ian.stokes@intel.com> wrote: >> >>> Yes, this was my thinking too. I don't think we should have a prohibition >>> against using "xxx", but I do usually see it in patches as something that >>> was left behind unintentionally. That said, my personal preference would >>> be to not have that often in the codebase, since it usually indicates >>> something half-baked, and more often than not, seems to be something no >>> one comes back to address later on. >> >> I'd agree, I guess there should be a valid explanation as to why it >> hasn't been implemented in the associated XXX comment. It'll >> definitely help to raise it as a point of discussion during the >> review. Sounds good to me. > > Great. Thanks! Because more voices of support are better - I am in favor of this change. I agree with the sentiment that checkpatch is meant to help bring things to light that humans sometimes glaze over (we all develop our own way of 'reading' code - for me I just don't *see* whitespace anymore, so checkpatch helps me self enforce it). I don't think I'd ever trust a machine, even one as snazzy as checkpatch, with true veto power over a change. In that vein, this helps draw the eye to "hey there's really something incomplete here - perhaps it is worth taking a closer look." Especially since I have been known to leave things behind - and having the checkpatch hook catch it for me early is better. All that long winded response to simply: Acked-by: Aaron Conole <aconole@redhat.com> On a side note - I was really happy to see that the in-comment tracking worked for most of the cases I tried. The only one that didn't catch is: /* XXX : something here */ but I don't think it's worth trying to solve that :) > --Justin > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> On Jan 25, 2018, at 12:36 PM, Aaron Conole <aconole@redhat.com> wrote: > > Because more voices of support are better - I am in favor of this > change. > > I agree with the sentiment that checkpatch is meant to help bring things > to light that humans sometimes glaze over (we all develop our own way of > 'reading' code - for me I just don't *see* whitespace anymore, so > checkpatch helps me self enforce it). I don't think I'd ever trust a > machine, even one as snazzy as checkpatch, with true veto power over a > change. In that vein, this helps draw the eye to "hey there's really > something incomplete here - perhaps it is worth taking a closer look." > > Especially since I have been known to leave things behind - and having > the checkpatch hook catch it for me early is better. > > All that long winded response to simply: > > Acked-by: Aaron Conole <aconole@redhat.com> Thanks! I pushed this to master. > On a side note - I was really happy to see that the in-comment tracking > worked for most of the cases I tried. The only one that didn't catch > is: > > /* > XXX : something here > */ > > but I don't think it's worth trying to solve that :) Thanks. The patch that initiated it had a couple variations, which made me work harder on it than I probably would have. I agree that it has some room for improvement, though. :-) --Justin
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py index 33feb6bddca1..42777e6fcb15 100755 --- a/utilities/checkpatch.py +++ b/utilities/checkpatch.py @@ -1,5 +1,6 @@ #!/usr/bin/env python # Copyright (c) 2016, 2017 Red Hat, Inc. +# Copyright (c) 2018 Nicira, Inc. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -95,9 +96,11 @@ __regex_ends_with_bracket = \ re.compile(r'[^\s]\) {(\s+/\*[\s\Sa-zA-Z0-9\.,\?\*/+-]*)?$') __regex_ptr_declaration_missing_whitespace = re.compile(r'[a-zA-Z0-9]\*[^*]') __regex_is_comment_line = re.compile(r'^\s*(/\*|\*\s)') +__regex_has_comment = re.compile(r'.*(/\*|\*\s)') __regex_trailing_operator = re.compile(r'^[^ ]* [^ ]*[?:]$') __regex_conditional_else_bracing = re.compile(r'^\s*else\s*{?$') __regex_conditional_else_bracing2 = re.compile(r'^\s*}\selse\s*$') +__regex_has_xxx_mark = re.compile(r'.*xxx.*', re.IGNORECASE) skip_leading_whitespace_check = False skip_trailing_whitespace_check = False @@ -213,11 +216,19 @@ def is_comment_line(line): """Returns TRUE if the current line is part of a block comment.""" return __regex_is_comment_line.match(line) is not None +def has_comment(line): + """Returns TRUE if the current line contains a comment or is part of + a block comment.""" + return __regex_has_comment.match(line) is not None def trailing_operator(line): """Returns TRUE if the current line ends with an operatorsuch as ? or :""" return __regex_trailing_operator.match(line) is not None +def has_xxx_mark(line): + """Returns TRUE if the current line contains 'xxx'.""" + return __regex_has_xxx_mark.match(line) is not None + checks = [ {'regex': None, @@ -257,6 +268,11 @@ checks = [ 'check': lambda x: trailing_operator(x), 'print': lambda: print_error("Line has '?' or ':' operator at end of line")}, + + {'regex': '(\.c|\.h)(\.in)?$', 'match_name': None, + 'prereq': lambda x: has_comment(x), + 'check': lambda x: has_xxx_mark(x), + 'print': lambda: print_warning("Comment with 'xxx' marker")}, ]
"xxx" is often used to indicate items that the developer wanted to look at again before committing. Flag those as a warning. Signed-off-by: Justin Pettit <jpettit@ovn.org> --- utilities/checkpatch.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)