[ovs-dev,v3,2/4] checkpatch: Add rule to check for hardcoded table numbers.

To avoid issues with hardcoded table numbers in future add rule
into check patch. The rule is only warning because there are still
legitimate use cases and not everything can be abstracted.

Signed-off-by: Ales Musil <amusil@redhat.com>
---
 utilities/checkpatch.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Thanks Ales,

Acked-by: Mark Michelson <mmichels@redhat.com>

On 2/8/24 13:17, Ales Musil wrote:
> To avoid issues with hardcoded table numbers in future add rule
> into check patch. The rule is only warning because there are still
> legitimate use cases and not everything can be abstracted.
> 
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---
>   utilities/checkpatch.py | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 52d3fa845..9f7a48c7c 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -202,6 +202,7 @@ __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' %
>                                  __parenthesized_constructs)
>   __regex_nonascii_characters = re.compile("[^\u0000-\u007f]")
>   __regex_efgrep = re.compile(r'.*[ef]grep.*$')
> +__regex_hardcoded_table = re.compile(r'(table=[0-9]+)|(resubmit\(,[0-9]+\))')
>   
>   skip_leading_whitespace_check = False
>   skip_trailing_whitespace_check = False
> @@ -371,6 +372,10 @@ def has_efgrep(line):
>       """Returns TRUE if the current line contains 'egrep' or 'fgrep'."""
>       return __regex_efgrep.match(line) is not None
>   
> +def has_hardcoded_table(line):
> +    """Return TRUE if the current line contains table=<NUMBER> or
> +       resubmit(,<NUMBER>)"""
> +    return __regex_hardcoded_table.match(line) is not None
>   
>   def filter_comments(current_line, keep=False):
>       """remove all of the c-style comments in a line"""
> @@ -656,6 +661,13 @@ checks = [
>        'check': lambda x: has_efgrep(x),
>        'print':
>        lambda: print_error("grep -E/-F should be used instead of egrep/fgrep")},
> +
> +    {'regex': r'\.at$', 'match_name': None,
> +     'check': lambda x: has_hardcoded_table(x),
> +     'print':
> +         lambda: print_warning("Use of hardcoded table=<NUMBER> or"
> +                               " resubmit=(,<NUMBER>) is discouraged in tests."
> +                               " Consider using MACRO instead.")},
>   ]
>   
>

On 2/8/24 19:17, Ales Musil wrote:
> To avoid issues with hardcoded table numbers in future add rule
> into check patch. The rule is only warning because there are still
> legitimate use cases and not everything can be abstracted.
> 
> Signed-off-by: Ales Musil <amusil@redhat.com>
> ---

Hi Ales,

>  utilities/checkpatch.py | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 

Would it be possible to add a test in checkpatch.at too?

> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 52d3fa845..9f7a48c7c 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -202,6 +202,7 @@ __regex_if_macros = re.compile(r'^ +(%s) \([\S]([\s\S]+[\S])*\) { +\\' %
>                                 __parenthesized_constructs)
>  __regex_nonascii_characters = re.compile("[^\u0000-\u007f]")
>  __regex_efgrep = re.compile(r'.*[ef]grep.*$')
> +__regex_hardcoded_table = re.compile(r'(table=[0-9]+)|(resubmit\(,[0-9]+\))')

This won't match on lines that include "table=XYZ" or "resubmit(,XYZ)"
but start with something else.

Should it be something like this?

__regex_hardcoded_table = re.compile(r'.*(table=[0-9]+)|(resubmit\(,[0-9]+\))')

>  
>  skip_leading_whitespace_check = False
>  skip_trailing_whitespace_check = False
> @@ -371,6 +372,10 @@ def has_efgrep(line):
>      """Returns TRUE if the current line contains 'egrep' or 'fgrep'."""
>      return __regex_efgrep.match(line) is not None
>  
> +def has_hardcoded_table(line):
> +    """Return TRUE if the current line contains table=<NUMBER> or
> +       resubmit(,<NUMBER>)"""
> +    return __regex_hardcoded_table.match(line) is not None
>  
>  def filter_comments(current_line, keep=False):
>      """remove all of the c-style comments in a line"""
> @@ -656,6 +661,13 @@ checks = [
>       'check': lambda x: has_efgrep(x),
>       'print':
>       lambda: print_error("grep -E/-F should be used instead of egrep/fgrep")},
> +
> +    {'regex': r'\.at$', 'match_name': None,
> +     'check': lambda x: has_hardcoded_table(x),
> +     'print':
> +         lambda: print_warning("Use of hardcoded table=<NUMBER> or"
> +                               " resubmit=(,<NUMBER>) is discouraged in tests."
> +                               " Consider using MACRO instead.")},
>  ]
>  
>  

Thanks,
Dumitru

On Mon, Feb 12, 2024 at 1:30 PM Dumitru Ceara <dceara@redhat.com> wrote:

> On 2/8/24 19:17, Ales Musil wrote:
> > To avoid issues with hardcoded table numbers in future add rule
> > into check patch. The rule is only warning because there are still
> > legitimate use cases and not everything can be abstracted.
> >
> > Signed-off-by: Ales Musil <amusil@redhat.com>
> > ---
>
> Hi Ales,
>

Hi Dumitru,

thank you for the review.


>
> >  utilities/checkpatch.py | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
>
> Would it be possible to add a test in checkpatch.at too?
>

Will do in v4.


>
> > diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> > index 52d3fa845..9f7a48c7c 100755
> > --- a/utilities/checkpatch.py
> > +++ b/utilities/checkpatch.py
> > @@ -202,6 +202,7 @@ __regex_if_macros = re.compile(r'^ +(%s)
> \([\S]([\s\S]+[\S])*\) { +\\' %
> >                                 __parenthesized_constructs)
> >  __regex_nonascii_characters = re.compile("[^\u0000-\u007f]")
> >  __regex_efgrep = re.compile(r'.*[ef]grep.*$')
> > +__regex_hardcoded_table =
> re.compile(r'(table=[0-9]+)|(resubmit\(,[0-9]+\))')
>
> This won't match on lines that include "table=XYZ" or "resubmit(,XYZ)"
> but start with something else.
>
> Should it be something like this?
>
> __regex_hardcoded_table =
> re.compile(r'.*(table=[0-9]+)|(resubmit\(,[0-9]+\))')
>

Yeah most likely, I didn't realize that python match is only from the start
even without the "^" specifier.


>
> >
> >  skip_leading_whitespace_check = False
> >  skip_trailing_whitespace_check = False
> > @@ -371,6 +372,10 @@ def has_efgrep(line):
> >      """Returns TRUE if the current line contains 'egrep' or 'fgrep'."""
> >      return __regex_efgrep.match(line) is not None
> >
> > +def has_hardcoded_table(line):
> > +    """Return TRUE if the current line contains table=<NUMBER> or
> > +       resubmit(,<NUMBER>)"""
> > +    return __regex_hardcoded_table.match(line) is not None
> >
> >  def filter_comments(current_line, keep=False):
> >      """remove all of the c-style comments in a line"""
> > @@ -656,6 +661,13 @@ checks = [
> >       'check': lambda x: has_efgrep(x),
> >       'print':
> >       lambda: print_error("grep -E/-F should be used instead of
> egrep/fgrep")},
> > +
> > +    {'regex': r'\.at$', 'match_name': None,
> > +     'check': lambda x: has_hardcoded_table(x),
> > +     'print':
> > +         lambda: print_warning("Use of hardcoded table=<NUMBER> or"
> > +                               " resubmit=(,<NUMBER>) is discouraged in
> tests."
> > +                               " Consider using MACRO instead.")},
> >  ]
> >
> >
>
> Thanks,
> Dumitru
>
>
Thanks.
Ales