| Message ID | 20240605135138.2872753-1-amorenoz@redhat.com |
|---|---|
| State | Accepted |
| Commit | 35e647051f984eeb305f76524f69a8bbf8c3933a |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev,v2] checkpatch: Don't warn on pointer to pointer. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
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 <amorenoz@redhat.com> Acked-by: Simon Horman <horms@ovn.org>
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 <amorenoz@redhat.com> Thanks for fixing this Adrian. Acked-by: Eelco Chaudron <echaudro@redhat.com>
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 <amorenoz@redhat.com> > > Thanks for fixing this Adrian. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> >
On Fri, Jun 07, 2024 at 07:06:21AM +0000, 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 <amorenoz@redhat.com> > > > > Thanks for fixing this Adrian. > > > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Jun 07, 2024 at 12:03:15PM +0100, Simon Horman wrote: > On Fri, Jun 07, 2024 at 07:06:21AM +0000, 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
Adrian Moreno <amorenoz@redhat.com> 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 <amorenoz@redhat.com> > --- 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'[^" \-(]\-[^"\->;]',
Aaron Conole <aconole@redhat.com> writes: > Adrian Moreno <amorenoz@redhat.com> 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 <amorenoz@redhat.com> >> --- > > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Mon, Jun 10, 2024 at 10:56:10AM -0400, Aaron Conole wrote: > Aaron Conole <aconole@redhat.com> writes: > > > Adrian Moreno <amorenoz@redhat.com> 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 <amorenoz@redhat.com> > >> --- > > > > 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? ...
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'[^" \-(]\-[^"\->;]',
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 <amorenoz@redhat.com> --- tests/checkpatch.at | 25 +++++++++++++++++++++++++ utilities/checkpatch.py | 2 +- 2 files changed, 26 insertions(+), 1 deletion(-)