On 24 May 2022, at 16:29, Peng He wrote:

> Signed-off-by: Peng He <[email protected]>
> Signed-off-by: Peng He <[email protected]>

This patch is making existing test fail:

 10: checkpatch - sign-offs                          ok
 11: checkpatch - parenthesized constructs           FAILED (checkpatch.at:27)
 12: checkpatch - catastrophic backtracking          FAILED (checkpatch.at:27)
 13: checkpatch - parenthesized constructs - for     FAILED (checkpatch.at:27)
 14: checkpatch - comments                           FAILED (checkpatch.at:27)
 15: checkpatch - whitespace around operator         FAILED (checkpatch.at:27)
 16: checkpatch - check misuse APIs                  ok
 17: checkpatch - whitespace around cast             FAILED (checkpatch.at:27)
 18: checkpatch - malformed tags                     ok
 19: checkpatch - Unicode code                       FAILED (checkpatch.at:27)

See also the fix below, which was what I had in mind asking for a single macro.

> ---
>  tests/checkpatch.at     | 30 ++++++++++++++++++++++++++----
>  utilities/checkpatch.py | 36 +++++++++++++++++++++++++++++++++---
>  2 files changed, 59 insertions(+), 7 deletions(-)
>
> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
> index fadb625e9..4dceb8b26 100755
> --- a/tests/checkpatch.at
> +++ b/tests/checkpatch.at
> @@ -159,17 +159,21 @@ try_checkpatch \
>  AT_CLEANUP
>
>
> -m4_define([COMMON_PATCH_HEADER], [dnl
> +m4_define([_COMMON_PATCH_HEADER], [dnl
>      Author: A
>
>      Signed-off-by: A
>      ---
> -    diff --git a/A.c b/A.c
> +    diff --git a/$1 b/$1
>      index 0000000..1111111 100644
> -    --- a/A.c
> -    +++ b/A.c
> +    --- a/$1
> +    +++ b/$1
>      @@ -1,1 +1,1 @@])
>
> +m4_define([COMMON_PATCH_HEADER],
> +    [_COMMON_PATCH_HEADER([A.c])
> +])
> +


I was hoping for a more simple solution, i.e. if $1 is not present use A.c or 
else the value given.
This is a diff I used to also make all existing tests pass:

diff --git a/tests/checkpatch.at b/tests/checkpatch.at
index 4dceb8b26..5b326b083 100755
--- a/tests/checkpatch.at
+++ b/tests/checkpatch.at
@@ -159,24 +159,20 @@ try_checkpatch \
 AT_CLEANUP


-m4_define([_COMMON_PATCH_HEADER], [dnl
+m4_define([COMMON_PATCH_HEADER], [dnl
     Author: A

     Signed-off-by: A
     ---
-    diff --git a/$1 b/$1
+    diff --git a/$([[ ! -z "$1" ]] && echo "$1" || echo "A.c") b/$([[ ! -z 
"$1" ]] && echo "$1" || echo "A.c")
     index 0000000..1111111 100644
-    --- a/$1
-    +++ b/$1
+    --- a/$([[ ! -z "$1" ]] && echo "$1" || echo "A.c")
+    +++ b/$([[ ! -z "$1" ]] && echo "$1" || echo "A.c")
     @@ -1,1 +1,1 @@])

-m4_define([COMMON_PATCH_HEADER],
-    [_COMMON_PATCH_HEADER([A.c])
-])
-
-
 AT_SETUP([checkpatch - parenthesized constructs])
 for ctr in 'if' 'while' 'switch' 'HMAP_FOR_EACH' 'BITMAP_FOR_EACH_1'; do
+
 try_checkpatch \
    "COMMON_PATCH_HEADER
     +     $ctr (first_run) {
@@ -354,7 +350,7 @@ AT_CLEANUP

 AT_SETUP([checkpatch - check misuse APIs])
 try_checkpatch \
-   "_COMMON_PATCH_HEADER([a.c])
+   "COMMON_PATCH_HEADER([a.c])
     +     ovsrcu_barrier();
     " \
     "WARNING: Are you sure you need to use ovsrcu_barrier(), "\
@@ -364,7 +360,7 @@ try_checkpatch \
 "

 try_checkpatch \
-   "_COMMON_PATCH_HEADER([lib/ovs-rcu.c])
+   "COMMON_PATCH_HEADER([lib/ovs-rcu.c])
     +     ovsrcu_barrier();
     "

I was trying string substitution but that did not work with the framework, i.e. 
$(1:-A.c}.
Not sure if this $() is the best way, maybe there is a more elegant way.

>
>  AT_SETUP([checkpatch - parenthesized constructs])
>  for ctr in 'if' 'while' 'switch' 'HMAP_FOR_EACH' 'BITMAP_FOR_EACH_1'; do
> @@ -348,6 +352,24 @@ try_checkpatch \
>
>  AT_CLEANUP
>
> +AT_SETUP([checkpatch - check misuse APIs])
> +try_checkpatch \
> +   "_COMMON_PATCH_HEADER([a.c])

See above patch, make this just COMMON_PATCH_HEADER, with not parameters.

> +    +     ovsrcu_barrier();
> +    " \
> +    "WARNING: Are you sure you need to use ovsrcu_barrier(), "\
> +"in most cases ovsrcu_synchronize() will be fine?
> +    #8 FILE: a.c:1:

See above, make it A.c

> +         ovsrcu_barrier();
> +"
> +
> +try_checkpatch \
> +   "_COMMON_PATCH_HEADER([lib/ovs-rcu.c])
> +    +     ovsrcu_barrier();
> +    "
> +

> +AT_CLEANUP
> +
>  AT_SETUP([checkpatch - whitespace around cast])
>  try_checkpatch \
>     "COMMON_PATCH_HEADER
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 8c7faa419..8c02ac3ce 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -620,6 +620,10 @@ def regex_error_factory(description):
>      return lambda: print_error(description)
>
>
> +def regex_warn_factory(description):
> +    return lambda: print_warning(description)
> +
> +
>  std_functions = [
>          ('malloc', 'Use xmalloc() in place of malloc()'),
>          ('calloc', 'Use xcalloc() in place of calloc()'),
> @@ -636,6 +640,7 @@ std_functions = [
>          ('assert', 'Use ovs_assert() in place of assert()'),
>          ('error', 'Use ovs_error() in place of error()'),
>  ]
> +
>  checks += [
>      {'regex': r'(\.c|\.h)(\.in)?$',
>       'match_name': None,
> @@ -644,6 +649,21 @@ checks += [
>       'print': regex_error_factory(description)}
>      for (function_name, description) in std_functions]
>
> +easy_to_misuse_api = [
> +        ('ovsrcu_barrier',
> +            'lib/ovs-rcu.c',
> +            'Are you sure you need to use ovsrcu_barrier(), '
> +            'in most cases ovsrcu_synchronize() will be fine?'),
> +        ]
> +
> +checks += [
> +    {'regex': r'(\.c)(\.in)?$',
> +     'match_name': lambda x: x != location,
> +     'prereq': lambda x: not is_comment_line(x),
> +     'check': regex_function_factory(function_name),
> +     'print': regex_warn_factory(description)}
> +    for (function_name, location, description) in easy_to_misuse_api]
> +
>
>  def regex_operator_factory(operator):
>      regex = re.compile(r'^[^#][^"\']*[^ "]%s[^ "\'][^"]*' % operator)
> @@ -676,12 +696,22 @@ def get_file_type_checks(filename):
>      global checks
>      checkList = []
>      for check in checks:
> +        regex_check = True
> +        match_check = True
> +
>          if check['regex'] is None and check['match_name'] is None:
>              checkList.append(check)
> +            continue
> +
>          if check['regex'] is not None and \
> -           re.compile(check['regex']).search(filename) is not None:
> -            checkList.append(check)
> -        elif check['match_name'] is not None and 
> check['match_name'](filename):
> +           re.compile(check['regex']).search(filename) is None:
> +            regex_check = False
> +
> +        if check['match_name'] is not None and \
> +                not check['match_name'](filename):
> +            match_check = False
> +
> +        if regex_check and match_check:
>              checkList.append(check)
>      return checkList
>
> -- 
> 2.25.1

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to