Re: [ovs-dev] [PATCH v2] checkpatch: Don't warn on pointer to pointer.
On Mon, Jun 10, 2024 at 10:56:10AM -0400, Aaron Conole wrote: > Aaron Conole writes: > > > Adrian Moreno writes: > > > >> Current regexp used to check whitespaces around operators does not > >> consider that there can be more than one "*" together to express pointer > >> to pointer. > >> > >> As a result, false positive warnings are raised when the > >> patch contains a simple list of pointers, e.g: "char **errrp"). > >> > >> Fix the regexp to allow more than one consecutive "+" characters. > >> > >> Signed-off-by: Adrian Moreno > >> --- > > > > I'm not quite sure about the functionality of this patch. For example, > > this seems to pass just fine: > > > > list = * *bar; > > > > BUT I think the coding style shouldn't allow it. There's a question of > > how much / where we want to get the errors (after all, it's the same > > state we are in today of accepting these kinds of lines even when the > > checkpatch script gets it wrong). Obviously, the line above is a pretty > > extreme case, but I think there must be a better regex / matching > > criteria we can do for the asterisk rather than modifying this existing > > one. > > > > Maybe Ilya / Eelco feel different about it or have opinions. > > I just saw that this was applied already. Sorry for not waiting longer before applying. I agree that the concern you have raised is valid. Could we prepare a follow-up patch? ... ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] checkpatch: Don't warn on pointer to pointer.
Aaron Conole writes:
> Adrian Moreno writes:
>
>> Current regexp used to check whitespaces around operators does not
>> consider that there can be more than one "*" together to express pointer
>> to pointer.
>>
>> As a result, false positive warnings are raised when the
>> patch contains a simple list of pointers, e.g: "char **errrp").
>>
>> Fix the regexp to allow more than one consecutive "+" characters.
>>
>> Signed-off-by: Adrian Moreno
>> ---
>
> I'm not quite sure about the functionality of this patch. For example,
> this seems to pass just fine:
>
> list = * *bar;
>
> BUT I think the coding style shouldn't allow it. There's a question of
> how much / where we want to get the errors (after all, it's the same
> state we are in today of accepting these kinds of lines even when the
> checkpatch script gets it wrong). Obviously, the line above is a pretty
> extreme case, but I think there must be a better regex / matching
> criteria we can do for the asterisk rather than modifying this existing
> one.
>
> Maybe Ilya / Eelco feel different about it or have opinions.
I just saw that this was applied already.
>> tests/checkpatch.at | 25 +
>> utilities/checkpatch.py | 2 +-
>> 2 files changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
>> index caab2817b..34971c514 100755
>> --- a/tests/checkpatch.at
>> +++ b/tests/checkpatch.at
>> @@ -353,6 +353,31 @@ try_checkpatch \
>> if (--mcs->n_refs==0) {
>> "
>>
>> +try_checkpatch \
>> + "COMMON_PATCH_HEADER
>> ++char *string;
>> ++char **list;
>> ++char ***ptr_list;
>> +"
>> +
>> +try_checkpatch \
>> + "COMMON_PATCH_HEADER
>> ++char** list;
>> +" \
>> +"WARNING: Line lacks whitespace around operator
>> +#8 FILE: A.c:1:
>> +char** list;
>> +"
>> +
>> +try_checkpatch \
>> + "COMMON_PATCH_HEADER
>> ++char*** list;
>> +" \
>> +"WARNING: Line lacks whitespace around operator
>> +#8 FILE: A.c:1:
>> +char*** list;
>> +"
>> +
>> AT_CLEANUP
>>
>> AT_SETUP([checkpatch - check misuse APIs])
>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>> index 6b293770d..742a0bc47 100755
>> --- a/utilities/checkpatch.py
>> +++ b/utilities/checkpatch.py
>> @@ -739,7 +739,7 @@ infix_operators = \
>> '&=', '^=', '|=', '<<=', '>>=']] \
>> + [r'[^<" ]<[^=" ]',
>> r'[^\->" ]>[^=" ]',
>> - r'[^ !()/"]\*[^/]',
>> + r'[^ !()/"\*]\*+[^/]',
>> r'[^ !&()"]&',
>> r'[^" +(]\+[^"+;]',
>> r'[^" \-(]\-[^"\->;]',
>
> ___
> 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 v2] checkpatch: Don't warn on pointer to pointer.
Adrian Moreno writes:
> Current regexp used to check whitespaces around operators does not
> consider that there can be more than one "*" together to express pointer
> to pointer.
>
> As a result, false positive warnings are raised when the
> patch contains a simple list of pointers, e.g: "char **errrp").
>
> Fix the regexp to allow more than one consecutive "+" characters.
>
> Signed-off-by: Adrian Moreno
> ---
I'm not quite sure about the functionality of this patch. For example,
this seems to pass just fine:
list = * *bar;
BUT I think the coding style shouldn't allow it. There's a question of
how much / where we want to get the errors (after all, it's the same
state we are in today of accepting these kinds of lines even when the
checkpatch script gets it wrong). Obviously, the line above is a pretty
extreme case, but I think there must be a better regex / matching
criteria we can do for the asterisk rather than modifying this existing
one.
Maybe Ilya / Eelco feel different about it or have opinions.
> tests/checkpatch.at | 25 +
> utilities/checkpatch.py | 2 +-
> 2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/tests/checkpatch.at b/tests/checkpatch.at
> index caab2817b..34971c514 100755
> --- a/tests/checkpatch.at
> +++ b/tests/checkpatch.at
> @@ -353,6 +353,31 @@ try_checkpatch \
> if (--mcs->n_refs==0) {
> "
>
> +try_checkpatch \
> + "COMMON_PATCH_HEADER
> ++char *string;
> ++char **list;
> ++char ***ptr_list;
> +"
> +
> +try_checkpatch \
> + "COMMON_PATCH_HEADER
> ++char** list;
> +" \
> +"WARNING: Line lacks whitespace around operator
> +#8 FILE: A.c:1:
> +char** list;
> +"
> +
> +try_checkpatch \
> + "COMMON_PATCH_HEADER
> ++char*** list;
> +" \
> +"WARNING: Line lacks whitespace around operator
> +#8 FILE: A.c:1:
> +char*** list;
> +"
> +
> AT_CLEANUP
>
> AT_SETUP([checkpatch - check misuse APIs])
> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
> index 6b293770d..742a0bc47 100755
> --- a/utilities/checkpatch.py
> +++ b/utilities/checkpatch.py
> @@ -739,7 +739,7 @@ infix_operators = \
> '&=', '^=', '|=', '<<=', '>>=']] \
> + [r'[^<" ]<[^=" ]',
> r'[^\->" ]>[^=" ]',
> - r'[^ !()/"]\*[^/]',
> + r'[^ !()/"\*]\*+[^/]',
> r'[^ !&()"]&',
> r'[^" +(]\+[^"+;]',
> r'[^" \-(]\-[^"\->;]',
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] checkpatch: Don't warn on pointer to pointer.
On Fri, Jun 07, 2024 at 12:03:15PM +0100, Simon Horman wrote: > On Fri, Jun 07, 2024 at 07:06:21AM +, Adrián Moreno wrote: > > On Fri, Jun 07, 2024 at 08:57:11AM GMT, Eelco Chaudron wrote: > > > > > > > > > On 5 Jun 2024, at 15:51, Adrian Moreno wrote: > > > > > > > Current regexp used to check whitespaces around operators does not > > > > consider that there can be more than one "*" together to express pointer > > > > to pointer. > > > > > > > > As a result, false positive warnings are raised when the > > > > patch contains a simple list of pointers, e.g: "char **errrp"). > > > > > > > > Fix the regexp to allow more than one consecutive "+" characters. > > > > > > > > I just noticed "+" should actually be "*"! > > Thanks Adrián, > > I can fix that when applying. Thanks Adrián and Eelco, Applied to main. - checkpatch: Don't warn on pointer to pointer. https://github.com/openvswitch/ovs/commit/35e647051f98 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] checkpatch: Don't warn on pointer to pointer.
On Fri, Jun 07, 2024 at 07:06:21AM +, Adrián Moreno wrote: > On Fri, Jun 07, 2024 at 08:57:11AM GMT, Eelco Chaudron wrote: > > > > > > On 5 Jun 2024, at 15:51, Adrian Moreno wrote: > > > > > Current regexp used to check whitespaces around operators does not > > > consider that there can be more than one "*" together to express pointer > > > to pointer. > > > > > > As a result, false positive warnings are raised when the > > > patch contains a simple list of pointers, e.g: "char **errrp"). > > > > > > Fix the regexp to allow more than one consecutive "+" characters. > > > > > I just noticed "+" should actually be "*"! Thanks Adrián, I can fix that when applying. > > > Signed-off-by: Adrian Moreno > > > > Thanks for fixing this Adrian. > > > > Acked-by: Eelco Chaudron > > > > ___ > 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 v2] checkpatch: Don't warn on pointer to pointer.
On Fri, Jun 07, 2024 at 08:57:11AM GMT, Eelco Chaudron wrote: > > > On 5 Jun 2024, at 15:51, Adrian Moreno wrote: > > > Current regexp used to check whitespaces around operators does not > > consider that there can be more than one "*" together to express pointer > > to pointer. > > > > As a result, false positive warnings are raised when the > > patch contains a simple list of pointers, e.g: "char **errrp"). > > > > Fix the regexp to allow more than one consecutive "+" characters. > > I just noticed "+" should actually be "*"! > > Signed-off-by: Adrian Moreno > > Thanks for fixing this Adrian. > > Acked-by: Eelco Chaudron > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] checkpatch: Don't warn on pointer to pointer.
On 5 Jun 2024, at 15:51, Adrian Moreno wrote: > Current regexp used to check whitespaces around operators does not > consider that there can be more than one "*" together to express pointer > to pointer. > > As a result, false positive warnings are raised when the > patch contains a simple list of pointers, e.g: "char **errrp"). > > Fix the regexp to allow more than one consecutive "+" characters. > > Signed-off-by: Adrian Moreno Thanks for fixing this Adrian. Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] checkpatch: Don't warn on pointer to pointer.
On Wed, Jun 05, 2024 at 03:51:38PM +0200, Adrian Moreno wrote: > Current regexp used to check whitespaces around operators does not > consider that there can be more than one "*" together to express pointer > to pointer. > > As a result, false positive warnings are raised when the > patch contains a simple list of pointers, e.g: "char **errrp"). > > Fix the regexp to allow more than one consecutive "+" characters. > > Signed-off-by: Adrian Moreno Acked-by: Simon Horman ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] checkpatch: Don't warn on pointer to pointer.
Current regexp used to check whitespaces around operators does not
consider that there can be more than one "*" together to express pointer
to pointer.
As a result, false positive warnings are raised when the
patch contains a simple list of pointers, e.g: "char **errrp").
Fix the regexp to allow more than one consecutive "+" characters.
Signed-off-by: Adrian Moreno
---
tests/checkpatch.at | 25 +
utilities/checkpatch.py | 2 +-
2 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/tests/checkpatch.at b/tests/checkpatch.at
index caab2817b..34971c514 100755
--- a/tests/checkpatch.at
+++ b/tests/checkpatch.at
@@ -353,6 +353,31 @@ try_checkpatch \
if (--mcs->n_refs==0) {
"
+try_checkpatch \
+ "COMMON_PATCH_HEADER
++char *string;
++char **list;
++char ***ptr_list;
+"
+
+try_checkpatch \
+ "COMMON_PATCH_HEADER
++char** list;
+" \
+"WARNING: Line lacks whitespace around operator
+#8 FILE: A.c:1:
+char** list;
+"
+
+try_checkpatch \
+ "COMMON_PATCH_HEADER
++char*** list;
+" \
+"WARNING: Line lacks whitespace around operator
+#8 FILE: A.c:1:
+char*** list;
+"
+
AT_CLEANUP
AT_SETUP([checkpatch - check misuse APIs])
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 6b293770d..742a0bc47 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -739,7 +739,7 @@ infix_operators = \
'&=', '^=', '|=', '<<=', '>>=']] \
+ [r'[^<" ]<[^=" ]',
r'[^\->" ]>[^=" ]',
- r'[^ !()/"]\*[^/]',
+ r'[^ !()/"\*]\*+[^/]',
r'[^ !&()"]&',
r'[^" +(]\+[^"+;]',
r'[^" \-(]\-[^"\->;]',
--
2.45.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
