Re: [ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in comments.

[Justin Pettit]

> On Jan 25, 2018, at 12:36 PM, Aaron Conole  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 

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


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in comments.

[Aaron Conole]

Justin Pettit  writes:

>> On Jan 25, 2018, at 10:26 AM, Stokes, Ian  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 


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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in comments.

[Justin Pettit]

> On Jan 25, 2018, at 10:26 AM, Stokes, Ian  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



___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in comments.

[Stokes, Ian]

> > On Jan 24, 2018, at 9:14 AM, Ben Pfaff <b...@ovn.org> wrote:
> >
> > On Wed, Jan 24, 2018 at 01:55:18PM +, Stokes, Ian wrote:
> >>> -Original Message-
> >>> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> >>> boun...@openvswitch.org] On Behalf Of Justin Pettit
> >>> Sent: Wednesday, January 24, 2018 2:31 AM
> >>> To: d...@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
> 

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in comments.

[Justin Pettit]

> On Jan 24, 2018, at 9:14 AM, Ben Pfaff <b...@ovn.org> wrote:
> 
> On Wed, Jan 24, 2018 at 01:55:18PM +, Stokes, Ian wrote:
>>> -Original Message-
>>> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
>>> boun...@openvswitch.org] On Behalf Of Justin Pettit
>>> Sent: Wednesday, January 24, 2018 2:31 AM
>>> To: d...@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


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in comments.

[Ben Pfaff]

On Wed, Jan 24, 2018 at 01:55:18PM +, Stokes, Ian wrote:
> > -Original Message-
> > From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> > boun...@openvswitch.org] On Behalf Of Justin Pettit
> > Sent: Wednesday, January 24, 2018 2:31 AM
> > To: d...@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.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in comments.

[Stokes, Ian]

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org [mailto:ovs-dev-
> boun...@openvswitch.org] On Behalf Of Justin Pettit
> Sent: Wednesday, January 24, 2018 2:31 AM
> To: d...@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 <jpet...@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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] checkpatch.py: Add check for "xxx" in comments.

[Justin Pettit]

"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 
---
 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev