linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
* [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
@ 2020-07-06  8:08 Mrinal Pandey
  2020-07-06 19:50 ` Lukas Bulwahn
  0 siblings, 1 reply; 13+ messages in thread
From: Mrinal Pandey @ 2020-07-06  8:08 UTC (permalink / raw)
  To: lukas.bulwahn, Linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 1597 bytes --]

checkpatch.pl issues warnings on the commits
made to scripts/spelling.txt for new entries
of typos and their fixes. This commit adjusts
checkpatch not to complain about the same.

Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
---
 scripts/checkpatch.pl | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3cacc122c528..538776faa96d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2994,14 +2994,16 @@ sub process {
 			while ($rawline =~ /(?:^|[^a-z@])($misspellings)(?:\b|$|[^a-z@])/gi) {
 				my $typo = $1;
 				my $typo_fix = $spelling_fix{lc($typo)};
-				$typo_fix = ucfirst($typo_fix) if ($typo =~ /^[A-Z]/);
-				$typo_fix = uc($typo_fix) if ($typo =~ /^[A-Z]+$/);
-				my $msg_level = \&WARN;
-				$msg_level = \&CHK if ($file);
-				if (&{$msg_level}("TYPO_SPELLING",
-						  "'$typo' may be misspelled - perhaps '$typo_fix'?\n" . $herecurr) &&
-				    $fix) {
-					$fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($typo)($|[^A-Za-z@])/$1$typo_fix$3/;
+				if ("+$typo||$typo_fix" ne $rawline) {
+					$typo_fix = ucfirst($typo_fix) if ($typo =~ /^[A-Z]/);
+					$typo_fix = uc($typo_fix) if ($typo =~ /^[A-Z]+$/);
+					my $msg_level = \&WARN;
+					$msg_level = \&CHK if ($file);
+					if (&{$msg_level}("TYPO_SPELLING",
+							  "'$typo' may be misspelled - perhaps '$typo_fix'?\n" . $herecurr) &&
+					    $fix) {
+						$fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($typo)($|[^A-Za-z@])/$1$typo_fix$3/;
+					}
 				}
 			}
 		}
-- 
2.25.1


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
  2020-07-06  8:08 [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive Mrinal Pandey
@ 2020-07-06 19:50 ` Lukas Bulwahn
       [not found]   ` <CAD1=X6m06KWb3=VEGkFWpmmV=UC09ZYbj2qpAOyg6OPFM=8KqA@mail.gmail.com>
  0 siblings, 1 reply; 13+ messages in thread
From: Lukas Bulwahn @ 2020-07-06 19:50 UTC (permalink / raw)
  To: Mrinal Pandey; +Cc: Linux-kernel-mentees

On Mon, Jul 6, 2020 at 10:08 AM Mrinal Pandey <mrinalmni@gmail.com> wrote:
>
> checkpatch.pl issues warnings on the commits
> made to scripts/spelling.txt for new entries
> of typos and their fixes. This commit adjusts
> checkpatch not to complain about the same.
>
> Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> ---

How often does that issue appear? Can you use your checkpatch
evaluation to show that it is relevant?


Would not be easier to just exclude the check of this whole rule if
checking the file scripts/spelling.txt.
That would at least be much more understandable, than this complicated
indirection you propose below.

>  scripts/checkpatch.pl | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 3cacc122c528..538776faa96d 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2994,14 +2994,16 @@ sub process {
>                         while ($rawline =~ /(?:^|[^a-z@])($misspellings)(?:\b|$|[^a-z@])/gi) {
>                                 my $typo = $1;
>                                 my $typo_fix = $spelling_fix{lc($typo)};
> -                               $typo_fix = ucfirst($typo_fix) if ($typo =~ /^[A-Z]/);
> -                               $typo_fix = uc($typo_fix) if ($typo =~ /^[A-Z]+$/);
> -                               my $msg_level = \&WARN;
> -                               $msg_level = \&CHK if ($file);
> -                               if (&{$msg_level}("TYPO_SPELLING",
> -                                                 "'$typo' may be misspelled - perhaps '$typo_fix'?\n" . $herecurr) &&
> -                                   $fix) {
> -                                       $fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($typo)($|[^A-Za-z@])/$1$typo_fix$3/;
> +                               if ("+$typo||$typo_fix" ne $rawline) {
> +                                       $typo_fix = ucfirst($typo_fix) if ($typo =~ /^[A-Z]/);
> +                                       $typo_fix = uc($typo_fix) if ($typo =~ /^[A-Z]+$/);
> +                                       my $msg_level = \&WARN;
> +                                       $msg_level = \&CHK if ($file);
> +                                       if (&{$msg_level}("TYPO_SPELLING",
> +                                                         "'$typo' may be misspelled - perhaps '$typo_fix'?\n" . $herecurr) &&
> +                                           $fix) {
> +                                               $fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($typo)($|[^A-Za-z@])/$1$typo_fix$3/;
> +                                       }
>                                 }
>                         }
>                 }
> --
> 2.25.1
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
       [not found]       ` <CAD1=X6==MgT6jaNZ9PsPUqV1QTRFKguq9zvV+TtFSHyajFMjUg@mail.gmail.com>
@ 2020-07-09  5:03         ` Lukas Bulwahn
  2020-07-10 11:26           ` Mrinal Pandey
  0 siblings, 1 reply; 13+ messages in thread
From: Lukas Bulwahn @ 2020-07-09  5:03 UTC (permalink / raw)
  To: Mrinal Pandey; +Cc: Linux-kernel-mentees

On Thu, Jul 9, 2020 at 6:31 AM Mrinal Pandey <mrinalmni@gmail.com> wrote:
>
> On Tue, Jul 7, 2020 at 10:24 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>>
>> On Tue, Jul 7, 2020 at 10:22 AM Mrinal Pandey <mrinalmni@gmail.com> wrote:
>> >
>> >
>> >
>> > On Tue, Jul 7, 2020 at 1:20 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>> >>
>> >> On Mon, Jul 6, 2020 at 10:08 AM Mrinal Pandey <mrinalmni@gmail.com> wrote:
>> >> >
>> >> > checkpatch.pl issues warnings on the commits
>> >> > made to scripts/spelling.txt for new entries
>> >> > of typos and their fixes. This commit adjusts
>> >> > checkpatch not to complain about the same.
>> >> >
>> >> > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
>> >> > ---
>> >>
>> >> How often does that issue appear? Can you use your checkpatch
>> >> evaluation to show that it is relevant?
>> >
>>
>> How many commits to spelling.txt happened within the last year?
>
>
> Sir,
>
> I could find only commit to the file in the range 5.7 to 5.8.rc-1.
>>
>>
>> The patch might be accepted, but the reason is not that convincing.
>
>
> What do you suggest? Should I send it or not?
>

Let us keep that in the backlog for now, but not send it. If it is
only one single case among hundreds false positives, it is maybe not
the best to start with.
We might get to that one case here eventually, but let us start with
the more important and critical cases first.

>> Maybe you can find another class of false positives that happen more
>> often?
>
>
> Yes, I have a few other suggestions that I found occurring often and I'm still evaluating to find more:
> 1. In `.h` files, when we write a function prototype, the name of the function parameters are
> not required, only the data type is enough, checkpatch says to define the name of the parameters too.
> Issues a warning like - function definition argument '<arg>' should also have an identifier name
>

Okay, we need to discuss if that is a convention that developers care
about or not.

> 2. A very common warning is - Macros with complex values should be enclosed in parentheses
> which is correct sometimes but a false positive many times, for macros ending with `)` or
> macros like `#define var value` we probably don't need another pair of `()`
>

Agree, this might be worth refining in checkpatch as you described.

> 3. checkpatch complains about breaking a quoted string across lines but this is many a time
> necessary for readability and in most of the patches I saw the strings broken.
>

Tricky to really know what the best solution is here. It is a tradeoff
in both directions.
Let us put that aside for now.

> 4. There are many patches where checkpatch issues false positives regarding spaces before
> and after lines.
>
Why are they false positives?

> 5. The warning - EXPORT_SYMBOL(foo); should immediately follow its function/variable
> is falsely positive in many cases where the statement is correct but the script fails to identify it.
>

If the script does not detect that, it sounds like a bug.
This can be improved for checkpatch.pl.

> 6. While running checkpatch on a patch the following error was thrown to the console -
> Use of uninitialized value $1 in regexp compilation at ./scripts/checkpatch.pl line 2653.
> This could be fixed.
>

That looks pretty sure like a bug.

> Please let me know your views on these ideas.

I suggest we look into issue 5 and 6.

For Issue 5: Can you provide me (and the CC: the list) the list of
false positives (the commit hashes) you found for issue 5 on
EXPORT_SYMBOL? Can you also provide a short rationale/explanation for
each case that you considered a false positive?

For Issue 6: Can you provide me the commit hash that caused this
checkpatch.pl error? Then, we can reproduce and confirm that issue
probably simply with `git format-patch -1 $SHA |
./scripts/checkpatch.pl` and observe the bug and crash ourselves?

(I added linux-kernel-mentees@lists.linuxfoundation.org back to the
recipient list.)
Also, on sending emails: you started the thread on
linux-kernel-mentees@lists.linuxfoundation.org. All further replies
shall always include that list in To or CC, so that the email thread
is complete on the list.

At some point in this mail thread, you only replied to me but did not
have the list in the recipient list (in To or CC). That was wrong;
Please follow the rule stated above. I hope this point was already
taught on the LF Kernel Development Introduction course. Maybe you can
check the material once again and see if and where that was pointed
out in the course material?

Thanks, waiting for response,

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
  2020-07-09  5:03         ` Lukas Bulwahn
@ 2020-07-10 11:26           ` Mrinal Pandey
  2020-07-10 20:46             ` Shuah Khan
                               ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Mrinal Pandey @ 2020-07-10 11:26 UTC (permalink / raw)
  To: Lukas Bulwahn, Linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 5966 bytes --]

On Thu, Jul 9, 2020 at 10:33 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
wrote:

> On Thu, Jul 9, 2020 at 6:31 AM Mrinal Pandey <mrinalmni@gmail.com> wrote:
> >
> > On Tue, Jul 7, 2020 at 10:24 PM Lukas Bulwahn <lukas.bulwahn@gmail.com>
> wrote:
> >>
> >> On Tue, Jul 7, 2020 at 10:22 AM Mrinal Pandey <mrinalmni@gmail.com>
> wrote:
> >> >
> >> >
> >> >
> >> > On Tue, Jul 7, 2020 at 1:20 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
> wrote:
> >> >>
> >> >> On Mon, Jul 6, 2020 at 10:08 AM Mrinal Pandey <mrinalmni@gmail.com>
> wrote:
> >> >> >
> >> >> > checkpatch.pl issues warnings on the commits
> >> >> > made to scripts/spelling.txt for new entries
> >> >> > of typos and their fixes. This commit adjusts
> >> >> > checkpatch not to complain about the same.
> >> >> >
> >> >> > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com>
> >> >> > ---
> >> >>
> >> >> How often does that issue appear? Can you use your checkpatch
> >> >> evaluation to show that it is relevant?
> >> >
> >>
> >> How many commits to spelling.txt happened within the last year?
> >
> >
> > Sir,
> >
> > I could find only commit to the file in the range 5.7 to 5.8.rc-1.
> >>
> >>
> >> The patch might be accepted, but the reason is not that convincing.
> >
> >
> > What do you suggest? Should I send it or not?
> >
>
> Let us keep that in the backlog for now, but not send it. If it is
> only one single case among hundreds false positives, it is maybe not
> the best to start with.
> We might get to that one case here eventually, but let us start with
> the more important and critical cases first.
>

> >> Maybe you can find another class of false positives that happen more
> >> often?
> >
> >
> > Yes, I have a few other suggestions that I found occurring often and I'm
> still evaluating to find more:
> > 1. In `.h` files, when we write a function prototype, the name of the
> function parameters are
> > not required, only the data type is enough, checkpatch says to define
> the name of the parameters too.
> > Issues a warning like - function definition argument '<arg>' should also
> have an identifier name
> >
>
> Okay, we need to discuss if that is a convention that developers care
> about or not.
>

> > 2. A very common warning is - Macros with complex values should be
> enclosed in parentheses
> > which is correct sometimes but a false positive many times, for macros
> ending with `)` or
> > macros like `#define var value` we probably don't need another pair of
> `()`
> >
>
> Agree, this might be worth refining in checkpatch as you described.
>
> > 3. checkpatch complains about breaking a quoted string across lines but
> this is many a time
> > necessary for readability and in most of the patches I saw the strings
> broken.
> >
>
> Tricky to really know what the best solution is here. It is a tradeoff
> in both directions.
> Let us put that aside for now.
>
> > 4. There are many patches where checkpatch issues false positives
> regarding spaces before
> > and after lines.
> >
> Why are they false positives?
>

Sir,

The warning by checkpatch says - please, no spaces at the start of a line
but there are indeed no spaces before the line where this warning is issued.
There are multiple commits having this issue, two of them are
`acaab7335bd6` and `372b38ea5911`.

>
> > 5. The warning - EXPORT_SYMBOL(foo); should immediately follow its
> function/variable
> > is falsely positive in many cases where the statement is correct but the
> script fails to identify it.
> >
>
> If the script does not detect that, it sounds like a bug.
> This can be improved for checkpatch.pl.
>
> > 6. While running checkpatch on a patch the following error was thrown to
> the console -
> > Use of uninitialized value $1 in regexp compilation at ./scripts/
> checkpatch.pl line 2653.
> > This could be fixed.
> >
>
> That looks pretty sure like a bug.
>
> > Please let me know your views on these ideas.
>
> I suggest we look into issue 5 and 6.
>
> For Issue 5: Can you provide me (and the CC: the list) the list of
> false positives (the commit hashes) you found for issue 5 on
> EXPORT_SYMBOL?


Here are the commit hashes for which the warning is issued:
54505a1e2083
75d75b7a4d54
8084c99b9af6
bfdaf029c9c9
dfd402a4c4ba

Can you also provide a short rationale/explanation for
> each case that you considered a false positive?
>

In each case the `EXPORT_SYMBOL()` is correctly written and the
variable/function to be exported
is also inside the parentheses, still, we get the warning. Please let me
know if I am wrong here.

>
> For Issue 6: Can you provide me the commit hash that caused this
> checkpatch.pl error? Then, we can reproduce and confirm that issue
> probably simply with `git format-patch -1 $SHA |
> ./scripts/checkpatch.pl` and observe the bug and crash ourselves?
>

These are the commit hashes that crashed the checkpatch:
6b3e0e2e0461
19ce2321739d
059c6d68cfc5


>
> (I added linux-kernel-mentees@lists.linuxfoundation.org back to the
> recipient list.)
> Also, on sending emails: you started the thread on
> linux-kernel-mentees@lists.linuxfoundation.org. All further replies
> shall always include that list in To or CC, so that the email thread
> is complete on the list.
>
> At some point in this mail thread, you only replied to me but did not
> have the list in the recipient list (in To or CC). That was wrong;
> Please follow the rule stated above. I hope this point was already
> taught on the LF Kernel Development Introduction course. Maybe you can
> check the material once again and see if and where that was pointed
> out in the course material?
>

Sir, I apologize for not including the list in my previous replies.
Unfortunately, it slipped out of my mind.
I assure you it would not happen again. Also, Linux Kernel Mentorship wiki
says to CC the overall
program mentor Shuah Khan Ma'am on each contribution. Should I do it only
on the final patches or on
every mail I send?

Thank you.

>
> Thanks, waiting for response,
>
> Lukas
>

[-- Attachment #1.2: Type: text/html, Size: 9116 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
  2020-07-10 11:26           ` Mrinal Pandey
@ 2020-07-10 20:46             ` Shuah Khan
  2020-07-10 21:15               ` Lukas Bulwahn
  2020-07-10 21:31             ` Lukas Bulwahn
  2020-07-10 21:47             ` Lukas Bulwahn
  2 siblings, 1 reply; 13+ messages in thread
From: Shuah Khan @ 2020-07-10 20:46 UTC (permalink / raw)
  To: Mrinal Pandey, Lukas Bulwahn, Linux-kernel-mentees, Shuah Khan

On 7/10/20 5:26 AM, Mrinal Pandey wrote:
> On Thu, Jul 9, 2020 at 10:33 AM Lukas Bulwahn <lukas.bulwahn@gmail.com 
> <mailto:lukas.bulwahn@gmail.com>> wrote:
> 
>     On Thu, Jul 9, 2020 at 6:31 AM Mrinal Pandey <mrinalmni@gmail.com
>     <mailto:mrinalmni@gmail.com>> wrote:
>      >
>      > On Tue, Jul 7, 2020 at 10:24 PM Lukas Bulwahn
>     <lukas.bulwahn@gmail.com <mailto:lukas.bulwahn@gmail.com>> wrote:
>      >>
>      >> On Tue, Jul 7, 2020 at 10:22 AM Mrinal Pandey
>     <mrinalmni@gmail.com <mailto:mrinalmni@gmail.com>> wrote:
>      >> >
>      >> >
>      >> >
>      >> > On Tue, Jul 7, 2020 at 1:20 AM Lukas Bulwahn
>     <lukas.bulwahn@gmail.com <mailto:lukas.bulwahn@gmail.com>> wrote:
>      >> >>
>      >> >> On Mon, Jul 6, 2020 at 10:08 AM Mrinal Pandey
>     <mrinalmni@gmail.com <mailto:mrinalmni@gmail.com>> wrote:
>      >> >> >
>      >> >> > checkpatch.pl <http://checkpatch.pl> issues warnings on the
>     commits
>      >> >> > made to scripts/spelling.txt for new entries
>      >> >> > of typos and their fixes. This commit adjusts
>      >> >> > checkpatch not to complain about the same.
>      >> >> >
>      >> >> > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com
>     <mailto:mrinalmni@gmail.com>>
>      >> >> > ---
>      >> >>
>      >> >> How often does that issue appear? Can you use your checkpatch
>      >> >> evaluation to show that it is relevant?
>      >> >
>      >>
>      >> How many commits to spelling.txt happened within the last year?
>      >
>      >
>      > Sir,
>      >
>      > I could find only commit to the file in the range 5.7 to 5.8.rc-1.
>      >>
>      >>
>      >> The patch might be accepted, but the reason is not that convincing.
>      >
>      >
>      > What do you suggest? Should I send it or not?
>      >
> 
>     Let us keep that in the backlog for now, but not send it. If it is
>     only one single case among hundreds false positives, it is maybe not
>     the best to start with.
>     We might get to that one case here eventually, but let us start with
>     the more important and critical cases first.
> 
> 
>      >> Maybe you can find another class of false positives that happen more
>      >> often?
>      >
>      >
>      > Yes, I have a few other suggestions that I found occurring often
>     and I'm still evaluating to find more:
>      > 1. In `.h` files, when we write a function prototype, the name of
>     the function parameters are
>      > not required, only the data type is enough, checkpatch says to
>     define the name of the parameters too.
>      > Issues a warning like - function definition argument '<arg>'
>     should also have an identifier name
>      >
> 
>     Okay, we need to discuss if that is a convention that developers care
>     about or not.
> 
> 
>      > 2. A very common warning is - Macros with complex values should
>     be enclosed in parentheses
>      > which is correct sometimes but a false positive many times, for
>     macros ending with `)` or
>      > macros like `#define var value` we probably don't need another
>     pair of `()`
>      >
> 
>     Agree, this might be worth refining in checkpatch as you described.
> 
>      > 3. checkpatch complains about breaking a quoted string across
>     lines but this is many a time
>      > necessary for readability and in most of the patches I saw the
>     strings broken.
>      >
> 
>     Tricky to really know what the best solution is here. It is a tradeoff
>     in both directions.
>     Let us put that aside for now.
> 
>      > 4. There are many patches where checkpatch issues false positives
>     regarding spaces before
>      > and after lines.
>      >
>     Why are they false positives?
> 
> 
> Sir,
> 
> The warning by checkpatch says - please, no spaces at the start of a line
> but there are indeed no spaces before the line where this warning is issued.
> There are multiple commits having this issue, two of them are 
> `acaab7335bd6` and `372b38ea5911`.
> 
> 
>      > 5. The warning - EXPORT_SYMBOL(foo); should immediately follow
>     its function/variable
>      > is falsely positive in many cases where the statement is correct
>     but the script fails to identify it.
>      >
> 
>     If the script does not detect that, it sounds like a bug.
>     This can be improved for checkpatch.pl <http://checkpatch.pl>.
> 
>      > 6. While running checkpatch on a patch the following error was
>     thrown to the console -
>      > Use of uninitialized value $1 in regexp compilation at
>     ./scripts/checkpatch.pl <http://checkpatch.pl> line 2653.
>      > This could be fixed.
>      >
> 
>     That looks pretty sure like a bug.
> 
>      > Please let me know your views on these ideas.
> 
>     I suggest we look into issue 5 and 6.
> 
>     For Issue 5: Can you provide me (and the CC: the list) the list of
>     false positives (the commit hashes) you found for issue 5 on
>     EXPORT_SYMBOL?
> 
> 
> Here are the commit hashes for which the warning is issued:
> 54505a1e2083
> 75d75b7a4d54
> 8084c99b9af6
> bfdaf029c9c9
> dfd402a4c4ba
> 
>     Can you also provide a short rationale/explanation for
>     each case that you considered a false positive?
> 
> 
> In each case the `EXPORT_SYMBOL()` is correctly written and the 
> variable/function to be exported
> is also inside the parentheses, still, we get the warning. Please let me 
> know if I am wrong here.
> 
> 
>     For Issue 6: Can you provide me the commit hash that caused this
>     checkpatch.pl <http://checkpatch.pl> error? Then, we can reproduce
>     and confirm that issue
>     probably simply with `git format-patch -1 $SHA |
>     ./scripts/checkpatch.pl <http://checkpatch.pl>` and observe the bug
>     and crash ourselves?
> 
> 
> These are the commit hashes that crashed the checkpatch:
> 6b3e0e2e0461
> 19ce2321739d
> 059c6d68cfc5
> 
> 
>     (I added linux-kernel-mentees@lists.linuxfoundation.org
>     <mailto:linux-kernel-mentees@lists.linuxfoundation.org> back to the
>     recipient list.)
>     Also, on sending emails: you started the thread on
>     linux-kernel-mentees@lists.linuxfoundation.org
>     <mailto:linux-kernel-mentees@lists.linuxfoundation.org>. All further
>     replies
>     shall always include that list in To or CC, so that the email thread
>     is complete on the list.
> 
>     At some point in this mail thread, you only replied to me but did not
>     have the list in the recipient list (in To or CC). That was wrong;
>     Please follow the rule stated above. I hope this point was already
>     taught on the LF Kernel Development Introduction course. Maybe you can
>     check the material once again and see if and where that was pointed
>     out in the course material?
> 
> 
> Sir, I apologize for not including the list in my previous replies. 
> Unfortunately, it slipped out of my mind.
> I assure you it would not happen again. Also, Linux Kernel Mentorship 
> wiki says to CC the overall
> program mentor Shuah Khan Ma'am on each contribution. Should I do it 
> only on the final patches or on
> every mail I send?
> 

No worries. You are new and this is a learning process.

Please cc me on emails. You have to reply to the list when you respond
to patch reviews.

Please run get_maintainers.pl and include everybody get_maintainers.pl
suggests. Without doing so will add more work for you when you send
it to the community.

Please review LFD103 sections that talk about submitting patches and
also refer to the kernel documentation.

thanks,
-- Shuah
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
  2020-07-10 20:46             ` Shuah Khan
@ 2020-07-10 21:15               ` Lukas Bulwahn
  2020-07-11 13:40                 ` Mrinal Pandey
  0 siblings, 1 reply; 13+ messages in thread
From: Lukas Bulwahn @ 2020-07-10 21:15 UTC (permalink / raw)
  To: Shuah Khan; +Cc: Linux-kernel-mentees

On Fri, Jul 10, 2020 at 10:46 PM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 7/10/20 5:26 AM, Mrinal Pandey wrote:
> > On Thu, Jul 9, 2020 at 10:33 AM Lukas Bulwahn <lukas.bulwahn@gmail.com
> > <mailto:lukas.bulwahn@gmail.com>> wrote:
> >
> >     On Thu, Jul 9, 2020 at 6:31 AM Mrinal Pandey <mrinalmni@gmail.com
> >     <mailto:mrinalmni@gmail.com>> wrote:
> >      >
> >      > On Tue, Jul 7, 2020 at 10:24 PM Lukas Bulwahn
> >     <lukas.bulwahn@gmail.com <mailto:lukas.bulwahn@gmail.com>> wrote:
> >      >>
> >      >> On Tue, Jul 7, 2020 at 10:22 AM Mrinal Pandey
> >     <mrinalmni@gmail.com <mailto:mrinalmni@gmail.com>> wrote:
> >      >> >
> >      >> >
> >      >> >
> >      >> > On Tue, Jul 7, 2020 at 1:20 AM Lukas Bulwahn
> >     <lukas.bulwahn@gmail.com <mailto:lukas.bulwahn@gmail.com>> wrote:
> >      >> >>
> >      >> >> On Mon, Jul 6, 2020 at 10:08 AM Mrinal Pandey
> >     <mrinalmni@gmail.com <mailto:mrinalmni@gmail.com>> wrote:
> >      >> >> >
> >      >> >> > checkpatch.pl <http://checkpatch.pl> issues warnings on the
> >     commits
> >      >> >> > made to scripts/spelling.txt for new entries
> >      >> >> > of typos and their fixes. This commit adjusts
> >      >> >> > checkpatch not to complain about the same.
> >      >> >> >
> >      >> >> > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com
> >     <mailto:mrinalmni@gmail.com>>
> >      >> >> > ---
> >      >> >>
> >      >> >> How often does that issue appear? Can you use your checkpatch
> >      >> >> evaluation to show that it is relevant?
> >      >> >
> >      >>
> >      >> How many commits to spelling.txt happened within the last year?
> >      >
> >      >
> >      > Sir,
> >      >
> >      > I could find only commit to the file in the range 5.7 to 5.8.rc-1.
> >      >>
> >      >>
> >      >> The patch might be accepted, but the reason is not that convincing.
> >      >
> >      >
> >      > What do you suggest? Should I send it or not?
> >      >
> >
> >     Let us keep that in the backlog for now, but not send it. If it is
> >     only one single case among hundreds false positives, it is maybe not
> >     the best to start with.
> >     We might get to that one case here eventually, but let us start with
> >     the more important and critical cases first.
> >
> >
> >      >> Maybe you can find another class of false positives that happen more
> >      >> often?
> >      >
> >      >
> >      > Yes, I have a few other suggestions that I found occurring often
> >     and I'm still evaluating to find more:
> >      > 1. In `.h` files, when we write a function prototype, the name of
> >     the function parameters are
> >      > not required, only the data type is enough, checkpatch says to
> >     define the name of the parameters too.
> >      > Issues a warning like - function definition argument '<arg>'
> >     should also have an identifier name
> >      >
> >
> >     Okay, we need to discuss if that is a convention that developers care
> >     about or not.
> >
> >
> >      > 2. A very common warning is - Macros with complex values should
> >     be enclosed in parentheses
> >      > which is correct sometimes but a false positive many times, for
> >     macros ending with `)` or
> >      > macros like `#define var value` we probably don't need another
> >     pair of `()`
> >      >
> >
> >     Agree, this might be worth refining in checkpatch as you described.
> >
> >      > 3. checkpatch complains about breaking a quoted string across
> >     lines but this is many a time
> >      > necessary for readability and in most of the patches I saw the
> >     strings broken.
> >      >
> >
> >     Tricky to really know what the best solution is here. It is a tradeoff
> >     in both directions.
> >     Let us put that aside for now.
> >
> >      > 4. There are many patches where checkpatch issues false positives
> >     regarding spaces before
> >      > and after lines.
> >      >
> >     Why are they false positives?
> >
> >
> > Sir,
> >
> > The warning by checkpatch says - please, no spaces at the start of a line
> > but there are indeed no spaces before the line where this warning is issued.
> > There are multiple commits having this issue, two of them are
> > `acaab7335bd6` and `372b38ea5911`.
> >
> >
> >      > 5. The warning - EXPORT_SYMBOL(foo); should immediately follow
> >     its function/variable
> >      > is falsely positive in many cases where the statement is correct
> >     but the script fails to identify it.
> >      >
> >
> >     If the script does not detect that, it sounds like a bug.
> >     This can be improved for checkpatch.pl <http://checkpatch.pl>.
> >
> >      > 6. While running checkpatch on a patch the following error was
> >     thrown to the console -
> >      > Use of uninitialized value $1 in regexp compilation at
> >     ./scripts/checkpatch.pl <http://checkpatch.pl> line 2653.
> >      > This could be fixed.
> >      >
> >
> >     That looks pretty sure like a bug.
> >
> >      > Please let me know your views on these ideas.
> >
> >     I suggest we look into issue 5 and 6.
> >
> >     For Issue 5: Can you provide me (and the CC: the list) the list of
> >     false positives (the commit hashes) you found for issue 5 on
> >     EXPORT_SYMBOL?
> >
> >
> > Here are the commit hashes for which the warning is issued:
> > 54505a1e2083
> > 75d75b7a4d54
> > 8084c99b9af6
> > bfdaf029c9c9
> > dfd402a4c4ba
> >
> >     Can you also provide a short rationale/explanation for
> >     each case that you considered a false positive?
> >
> >
> > In each case the `EXPORT_SYMBOL()` is correctly written and the
> > variable/function to be exported
> > is also inside the parentheses, still, we get the warning. Please let me
> > know if I am wrong here.
> >
> >
> >     For Issue 6: Can you provide me the commit hash that caused this
> >     checkpatch.pl <http://checkpatch.pl> error? Then, we can reproduce
> >     and confirm that issue
> >     probably simply with `git format-patch -1 $SHA |
> >     ./scripts/checkpatch.pl <http://checkpatch.pl>` and observe the bug
> >     and crash ourselves?
> >
> >
> > These are the commit hashes that crashed the checkpatch:
> > 6b3e0e2e0461
> > 19ce2321739d
> > 059c6d68cfc5
> >
> >
> >     (I added linux-kernel-mentees@lists.linuxfoundation.org
> >     <mailto:linux-kernel-mentees@lists.linuxfoundation.org> back to the
> >     recipient list.)
> >     Also, on sending emails: you started the thread on
> >     linux-kernel-mentees@lists.linuxfoundation.org
> >     <mailto:linux-kernel-mentees@lists.linuxfoundation.org>. All further
> >     replies
> >     shall always include that list in To or CC, so that the email thread
> >     is complete on the list.
> >
> >     At some point in this mail thread, you only replied to me but did not
> >     have the list in the recipient list (in To or CC). That was wrong;
> >     Please follow the rule stated above. I hope this point was already
> >     taught on the LF Kernel Development Introduction course. Maybe you can
> >     check the material once again and see if and where that was pointed
> >     out in the course material?
> >
> >
> > Sir, I apologize for not including the list in my previous replies.
> > Unfortunately, it slipped out of my mind.
> > I assure you it would not happen again. Also, Linux Kernel Mentorship
> > wiki says to CC the overall
> > program mentor Shuah Khan Ma'am on each contribution. Should I do it
> > only on the final patches or on
> > every mail I send?
> >
>
> No worries. You are new and this is a learning process.
>
> Please cc me on emails. You have to reply to the list when you respond
> to patch reviews.
>
> Please run get_maintainers.pl and include everybody get_maintainers.pl
> suggests. Without doing so will add more work for you when you send
> it to the community.
>

Mrinal, please first send these suggested patches only to me, Shuah
and the linux-kernel-mentees list for reviewing.

If I am okay with a specific patch, I will let you know to then send
the patch to everybody get_maintainers.pl suggest, which will be for
the patches we discuss:

Andy Whitcroft <apw@canonical.com> (maintainer:CHECKPATCH)
Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
linux-kernel@vger.kernel.org (open list)

I want to make sure that I agree with the patch before sending it to
Andy and Joe.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
  2020-07-10 11:26           ` Mrinal Pandey
  2020-07-10 20:46             ` Shuah Khan
@ 2020-07-10 21:31             ` Lukas Bulwahn
  2020-07-11 13:37               ` Mrinal Pandey
  2020-07-10 21:47             ` Lukas Bulwahn
  2 siblings, 1 reply; 13+ messages in thread
From: Lukas Bulwahn @ 2020-07-10 21:31 UTC (permalink / raw)
  To: Mrinal Pandey; +Cc: Linux-kernel-mentees

>>
>> For Issue 6: Can you provide me the commit hash that caused this
>> checkpatch.pl error? Then, we can reproduce and confirm that issue
>> probably simply with `git format-patch -1 $SHA |
>> ./scripts/checkpatch.pl` and observe the bug and crash ourselves?
>
>
> These are the commit hashes that crashed the checkpatch:
> 6b3e0e2e0461
> 19ce2321739d
> 059c6d68cfc5
>

Okay, I checked the output of checkpatch.pl on those three commits and
I can confirm that checkpatch.pl warns during its own execution with:

Use of uninitialized value $1 in regexp compilation at
./scripts/checkpatch.pl line 2638.

Mrinal, can you debug and find out why and what specifically in those
patches cause this warning in line 2638?

Also, what is the intent and when was this introduced to
checkpatch.pl? It could be that it never worked since it was
introduced or that it broke due to some refactoring. Can you find what
happened in this case here?

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
  2020-07-10 11:26           ` Mrinal Pandey
  2020-07-10 20:46             ` Shuah Khan
  2020-07-10 21:31             ` Lukas Bulwahn
@ 2020-07-10 21:47             ` Lukas Bulwahn
  2020-07-14 14:56               ` Mrinal Pandey
  2 siblings, 1 reply; 13+ messages in thread
From: Lukas Bulwahn @ 2020-07-10 21:47 UTC (permalink / raw)
  To: Mrinal Pandey; +Cc: Linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 1027 bytes --]

>> For Issue 5: Can you provide me (and the CC: the list) the list of
>> false positives (the commit hashes) you found for issue 5 on
>> EXPORT_SYMBOL?
>
>
> Here are the commit hashes for which the warning is issued:
> 54505a1e2083
> 75d75b7a4d54
> 8084c99b9af6
> bfdaf029c9c9
> dfd402a4c4ba
>
>> Can you also provide a short rationale/explanation for
>> each case that you considered a false positive?
>
>
> In each case the `EXPORT_SYMBOL()` is correctly written and the
variable/function to be exported
> is also inside the parentheses, still, we get the warning. Please let me
know if I am wrong here.

I checked those warnings. Some of the patches are good cases to check if we
can improve the heuristics on EXPORT_SYMBOL().

E.g., commit bfdaf029c9c9 ("ia64: turn csum_partial_copy_from_user() into
csum_and_copy_from_user()") looks sound to me, and checkpatch.pl should not
really warn about that.

Mrinal, maybe you can find out why checkpatch.pl believes that this case is
wrong and needs to be warned about?


Lukas

[-- Attachment #1.2: Type: text/html, Size: 1386 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
  2020-07-10 21:31             ` Lukas Bulwahn
@ 2020-07-11 13:37               ` Mrinal Pandey
  0 siblings, 0 replies; 13+ messages in thread
From: Mrinal Pandey @ 2020-07-11 13:37 UTC (permalink / raw)
  To: Lukas Bulwahn, Shuah Khan, Linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 2017 bytes --]

On Sat, Jul 11, 2020 at 3:01 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
wrote:

> >>
> >> For Issue 6: Can you provide me the commit hash that caused this
> >> checkpatch.pl error? Then, we can reproduce and confirm that issue
> >> probably simply with `git format-patch -1 $SHA |
> >> ./scripts/checkpatch.pl` and observe the bug and crash ourselves?
> >
> >
> > These are the commit hashes that crashed the checkpatch:
> > 6b3e0e2e0461
> > 19ce2321739d
> > 059c6d68cfc5
> >
>
> Okay, I checked the output of checkpatch.pl on those three commits and
> I can confirm that checkpatch.pl warns during its own execution with:
>
> Use of uninitialized value $1 in regexp compilation at
> ./scripts/checkpatch.pl line 2638.
>
> Mrinal, can you debug and find out why and what specifically in those
> patches cause this warning in line 2638?
>

Sir,

The block in checkpatch where this issue appears is triggered only when
someone
writes a commit message that contains diff content. This is a rare event
hence the issue
in the block shows up only for a few specific commits.

>
> Also, what is the intent and when was this introduced to
> checkpatch.pl? It could be that it never worked since it was
> introduced or that it broke due to some refactoring. Can you find what
> happened in this case here?


The code which causes the issue is:
$line =~ m@^\s+diff\b.*a/[\w/]+@ && $line =~ m@
^\s+diff\b.*a/([\w/]+)\s+b/$1\b@

I believe the intent was to use the "capture groups ( ... )" of Perl and
then use `$1`, which
would have contained the previous match, in the regex expression. Somehow,
the capture
group is used in the same expression as `$1` leading `$1` to remain
uninitialized, i.e. ([\w/]+) followed by $1.
In my opinion, the ( ... ) should be used in the previous expression
thereby initializing `$1` to the required value.
When I do this change and run the script again, the error vanishes.
Please let me know if what I say sounds reasonable so that I will continue
with sending this patch to you.

>
> Lukas
>

[-- Attachment #1.2: Type: text/html, Size: 3302 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
  2020-07-10 21:15               ` Lukas Bulwahn
@ 2020-07-11 13:40                 ` Mrinal Pandey
  0 siblings, 0 replies; 13+ messages in thread
From: Mrinal Pandey @ 2020-07-11 13:40 UTC (permalink / raw)
  To: Lukas Bulwahn, Shuah Khan, Linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 8941 bytes --]

Okay, sir.

On Sat, Jul 11, 2020 at 2:46 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
wrote:

> On Fri, Jul 10, 2020 at 10:46 PM Shuah Khan <skhan@linuxfoundation.org>
> wrote:
> >
> > On 7/10/20 5:26 AM, Mrinal Pandey wrote:
> > > On Thu, Jul 9, 2020 at 10:33 AM Lukas Bulwahn <lukas.bulwahn@gmail.com
> > > <mailto:lukas.bulwahn@gmail.com>> wrote:
> > >
> > >     On Thu, Jul 9, 2020 at 6:31 AM Mrinal Pandey <mrinalmni@gmail.com
> > >     <mailto:mrinalmni@gmail.com>> wrote:
> > >      >
> > >      > On Tue, Jul 7, 2020 at 10:24 PM Lukas Bulwahn
> > >     <lukas.bulwahn@gmail.com <mailto:lukas.bulwahn@gmail.com>> wrote:
> > >      >>
> > >      >> On Tue, Jul 7, 2020 at 10:22 AM Mrinal Pandey
> > >     <mrinalmni@gmail.com <mailto:mrinalmni@gmail.com>> wrote:
> > >      >> >
> > >      >> >
> > >      >> >
> > >      >> > On Tue, Jul 7, 2020 at 1:20 AM Lukas Bulwahn
> > >     <lukas.bulwahn@gmail.com <mailto:lukas.bulwahn@gmail.com>> wrote:
> > >      >> >>
> > >      >> >> On Mon, Jul 6, 2020 at 10:08 AM Mrinal Pandey
> > >     <mrinalmni@gmail.com <mailto:mrinalmni@gmail.com>> wrote:
> > >      >> >> >
> > >      >> >> > checkpatch.pl <http://checkpatch.pl> issues warnings on
> the
> > >     commits
> > >      >> >> > made to scripts/spelling.txt for new entries
> > >      >> >> > of typos and their fixes. This commit adjusts
> > >      >> >> > checkpatch not to complain about the same.
> > >      >> >> >
> > >      >> >> > Signed-off-by: Mrinal Pandey <mrinalmni@gmail.com
> > >     <mailto:mrinalmni@gmail.com>>
> > >      >> >> > ---
> > >      >> >>
> > >      >> >> How often does that issue appear? Can you use your
> checkpatch
> > >      >> >> evaluation to show that it is relevant?
> > >      >> >
> > >      >>
> > >      >> How many commits to spelling.txt happened within the last year?
> > >      >
> > >      >
> > >      > Sir,
> > >      >
> > >      > I could find only commit to the file in the range 5.7 to
> 5.8.rc-1.
> > >      >>
> > >      >>
> > >      >> The patch might be accepted, but the reason is not that
> convincing.
> > >      >
> > >      >
> > >      > What do you suggest? Should I send it or not?
> > >      >
> > >
> > >     Let us keep that in the backlog for now, but not send it. If it is
> > >     only one single case among hundreds false positives, it is maybe
> not
> > >     the best to start with.
> > >     We might get to that one case here eventually, but let us start
> with
> > >     the more important and critical cases first.
> > >
> > >
> > >      >> Maybe you can find another class of false positives that
> happen more
> > >      >> often?
> > >      >
> > >      >
> > >      > Yes, I have a few other suggestions that I found occurring often
> > >     and I'm still evaluating to find more:
> > >      > 1. In `.h` files, when we write a function prototype, the name
> of
> > >     the function parameters are
> > >      > not required, only the data type is enough, checkpatch says to
> > >     define the name of the parameters too.
> > >      > Issues a warning like - function definition argument '<arg>'
> > >     should also have an identifier name
> > >      >
> > >
> > >     Okay, we need to discuss if that is a convention that developers
> care
> > >     about or not.
> > >
> > >
> > >      > 2. A very common warning is - Macros with complex values should
> > >     be enclosed in parentheses
> > >      > which is correct sometimes but a false positive many times, for
> > >     macros ending with `)` or
> > >      > macros like `#define var value` we probably don't need another
> > >     pair of `()`
> > >      >
> > >
> > >     Agree, this might be worth refining in checkpatch as you described.
> > >
> > >      > 3. checkpatch complains about breaking a quoted string across
> > >     lines but this is many a time
> > >      > necessary for readability and in most of the patches I saw the
> > >     strings broken.
> > >      >
> > >
> > >     Tricky to really know what the best solution is here. It is a
> tradeoff
> > >     in both directions.
> > >     Let us put that aside for now.
> > >
> > >      > 4. There are many patches where checkpatch issues false
> positives
> > >     regarding spaces before
> > >      > and after lines.
> > >      >
> > >     Why are they false positives?
> > >
> > >
> > > Sir,
> > >
> > > The warning by checkpatch says - please, no spaces at the start of a
> line
> > > but there are indeed no spaces before the line where this warning is
> issued.
> > > There are multiple commits having this issue, two of them are
> > > `acaab7335bd6` and `372b38ea5911`.
> > >
> > >
> > >      > 5. The warning - EXPORT_SYMBOL(foo); should immediately follow
> > >     its function/variable
> > >      > is falsely positive in many cases where the statement is correct
> > >     but the script fails to identify it.
> > >      >
> > >
> > >     If the script does not detect that, it sounds like a bug.
> > >     This can be improved for checkpatch.pl <http://checkpatch.pl>.
> > >
> > >      > 6. While running checkpatch on a patch the following error was
> > >     thrown to the console -
> > >      > Use of uninitialized value $1 in regexp compilation at
> > >     ./scripts/checkpatch.pl <http://checkpatch.pl> line 2653.
> > >      > This could be fixed.
> > >      >
> > >
> > >     That looks pretty sure like a bug.
> > >
> > >      > Please let me know your views on these ideas.
> > >
> > >     I suggest we look into issue 5 and 6.
> > >
> > >     For Issue 5: Can you provide me (and the CC: the list) the list of
> > >     false positives (the commit hashes) you found for issue 5 on
> > >     EXPORT_SYMBOL?
> > >
> > >
> > > Here are the commit hashes for which the warning is issued:
> > > 54505a1e2083
> > > 75d75b7a4d54
> > > 8084c99b9af6
> > > bfdaf029c9c9
> > > dfd402a4c4ba
> > >
> > >     Can you also provide a short rationale/explanation for
> > >     each case that you considered a false positive?
> > >
> > >
> > > In each case the `EXPORT_SYMBOL()` is correctly written and the
> > > variable/function to be exported
> > > is also inside the parentheses, still, we get the warning. Please let
> me
> > > know if I am wrong here.
> > >
> > >
> > >     For Issue 6: Can you provide me the commit hash that caused this
> > >     checkpatch.pl <http://checkpatch.pl> error? Then, we can reproduce
> > >     and confirm that issue
> > >     probably simply with `git format-patch -1 $SHA |
> > >     ./scripts/checkpatch.pl <http://checkpatch.pl>` and observe the
> bug
> > >     and crash ourselves?
> > >
> > >
> > > These are the commit hashes that crashed the checkpatch:
> > > 6b3e0e2e0461
> > > 19ce2321739d
> > > 059c6d68cfc5
> > >
> > >
> > >     (I added linux-kernel-mentees@lists.linuxfoundation.org
> > >     <mailto:linux-kernel-mentees@lists.linuxfoundation.org> back to
> the
> > >     recipient list.)
> > >     Also, on sending emails: you started the thread on
> > >     linux-kernel-mentees@lists.linuxfoundation.org
> > >     <mailto:linux-kernel-mentees@lists.linuxfoundation.org>. All
> further
> > >     replies
> > >     shall always include that list in To or CC, so that the email
> thread
> > >     is complete on the list.
> > >
> > >     At some point in this mail thread, you only replied to me but did
> not
> > >     have the list in the recipient list (in To or CC). That was wrong;
> > >     Please follow the rule stated above. I hope this point was already
> > >     taught on the LF Kernel Development Introduction course. Maybe you
> can
> > >     check the material once again and see if and where that was pointed
> > >     out in the course material?
> > >
> > >
> > > Sir, I apologize for not including the list in my previous replies.
> > > Unfortunately, it slipped out of my mind.
> > > I assure you it would not happen again. Also, Linux Kernel Mentorship
> > > wiki says to CC the overall
> > > program mentor Shuah Khan Ma'am on each contribution. Should I do it
> > > only on the final patches or on
> > > every mail I send?
> > >
> >
> > No worries. You are new and this is a learning process.
> >
> > Please cc me on emails. You have to reply to the list when you respond
> > to patch reviews.
> >
> > Please run get_maintainers.pl and include everybody get_maintainers.pl
> > suggests. Without doing so will add more work for you when you send
> > it to the community.
> >
>
> Mrinal, please first send these suggested patches only to me, Shuah
> and the linux-kernel-mentees list for reviewing.
>
> If I am okay with a specific patch, I will let you know to then send
> the patch to everybody get_maintainers.pl suggest, which will be for
> the patches we discuss:
>
> Andy Whitcroft <apw@canonical.com> (maintainer:CHECKPATCH)
> Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
> linux-kernel@vger.kernel.org (open list)
>
> I want to make sure that I agree with the patch before sending it to
> Andy and Joe.
>
> Lukas
>

[-- Attachment #1.2: Type: text/html, Size: 14272 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
  2020-07-10 21:47             ` Lukas Bulwahn
@ 2020-07-14 14:56               ` Mrinal Pandey
  2020-07-15  5:32                 ` Lukas Bulwahn
  0 siblings, 1 reply; 13+ messages in thread
From: Mrinal Pandey @ 2020-07-14 14:56 UTC (permalink / raw)
  To: Lukas Bulwahn, Shuah Khan, Linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 2217 bytes --]

On Sat, Jul 11, 2020 at 3:17 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
wrote:

>
> >> For Issue 5: Can you provide me (and the CC: the list) the list of
> >> false positives (the commit hashes) you found for issue 5 on
> >> EXPORT_SYMBOL?
> >
> >
> > Here are the commit hashes for which the warning is issued:
> > 54505a1e2083
> > 75d75b7a4d54
> > 8084c99b9af6
> > bfdaf029c9c9
> > dfd402a4c4ba
> >
> >> Can you also provide a short rationale/explanation for
> >> each case that you considered a false positive?
> >
> >
> > In each case the `EXPORT_SYMBOL()` is correctly written and the
> variable/function to be exported
> > is also inside the parentheses, still, we get the warning. Please let me
> know if I am wrong here.
>
> I checked those warnings. Some of the patches are good cases to check if
> we can improve the heuristics on EXPORT_SYMBOL().
>
> E.g., commit bfdaf029c9c9 ("ia64: turn csum_partial_copy_from_user() into
> csum_and_copy_from_user()") looks sound to me, and checkpatch.pl should
> not really warn about that.
>
> Mrinal, maybe you can find out why checkpatch.pl believes that this case
> is wrong and needs to be warned about?
>

Sir,

Commit `54505a1e2083`, `8084c99b9af6` and `bfdaf029c9c9` have style issues.
The documentation says to use
`EXPORT_SYMBOL()` just after the closing bracket `}` of the function, it is
exporting. In these commits `EXPORT SYMBOL()`
is either defined at a point later or has been defined after leaving a
blank line which is totally unneeded.

However, commits `dfd402a4c4ba` and `75d75b7a4d54` look fine to me and have
one thing in common, the
`EXPORT_SYMBOL()` in these is used inside of a macro. checkpatch shouldn't
warn about these.

I have noticed checkpatch denoting wrong line numbers for certain files for
several commits.
For example, for the above commit
`bfdaf029c9c9`, the following warning is issued:

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
#74: FILE: arch/ia64/lib/csum_partial_copy.c:122:
+EXPORT_SYMBOL(csum_and_copy_from_user);

but the file arch/ia64/lib/csum_partial_copy.c is only 113 lines long.
Please let me know if you can reproduce this.

Waiting for your insights.
Thank you.

>
>
> Lukas
>

[-- Attachment #1.2: Type: text/html, Size: 3287 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
  2020-07-14 14:56               ` Mrinal Pandey
@ 2020-07-15  5:32                 ` Lukas Bulwahn
  2020-07-18  7:00                   ` Mrinal Pandey
  0 siblings, 1 reply; 13+ messages in thread
From: Lukas Bulwahn @ 2020-07-15  5:32 UTC (permalink / raw)
  To: Mrinal Pandey; +Cc: Linux-kernel-mentees

[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]



On Tue, 14 Jul 2020, Mrinal Pandey wrote:

> 
> 
> On Sat, Jul 11, 2020 at 3:17 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> 
> >> For Issue 5: Can you provide me (and the CC: the list) the list of
> >> false positives (the commit hashes) you found for issue 5 on
> >> EXPORT_SYMBOL?
> >
> >
> > Here are the commit hashes for which the warning is issued:
> > 54505a1e2083
> > 75d75b7a4d54
> > 8084c99b9af6
> > bfdaf029c9c9
> > dfd402a4c4ba
> >
> >> Can you also provide a short rationale/explanation for
> >> each case that you considered a false positive?
> >
> >
> > In each case the `EXPORT_SYMBOL()` is correctly written and the variable/function to be exported
> > is also inside the parentheses, still, we get the warning. Please let me know if I am wrong here.
> 
> I checked those warnings. Some of the patches are good cases to check if we can improve the heuristics on
> EXPORT_SYMBOL().
> 
> E.g., commit bfdaf029c9c9 ("ia64: turn csum_partial_copy_from_user() into csum_and_copy_from_user()") looks
> sound to me, and checkpatch.pl should not really warn about that.
> 
> Mrinal, maybe you can find out why checkpatch.pl believes that this case is wrong and needs to be warned
> about?
> 
> 
> Sir,
> 
> Commit `54505a1e2083`, `8084c99b9af6` and `bfdaf029c9c9` have style issues. The documentation says to use
> `EXPORT_SYMBOL()` just after the closing bracket `}` of the function, it is exporting. In these commits `EXPORT
> SYMBOL()`
> is either defined at a point later or has been defined after leaving a blank line which is totally unneeded.
>

Possibly, a new line is okay; I think we would some statistics on the 
overall use of EXPORT_SYMBOL() to see if a new line is generally accepted 
or if it is really a rare case (<3%) and checkpatch.pl should keep 
warning about that.
 
> However, commits `dfd402a4c4ba` and `75d75b7a4d54` look fine to me and have one thing in common, the
> `EXPORT_SYMBOL()` in these is used inside of a macro. checkpatch shouldn't warn about these.
> 

That is probably tricky to handle with checkpatch.pl, though.

> I have noticed checkpatch denoting wrong line numbers for certain files for several commits.
> For example, for the above commit
> `bfdaf029c9c9`, the following warning is issued:
> 
> WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> #74: FILE: arch/ia64/lib/csum_partial_copy.c:122:
> +EXPORT_SYMBOL(csum_and_copy_from_user);
> 
> but the file arch/ia64/lib/csum_partial_copy.c is only 113 lines long.
> Please let me know if you can reproduce this.

Can you find out how the line number is computed?

Of course, checkpatch.pl is running on the patch, not on any specific 
file. So, in general, you really do not know how long the file was that 
the author based the patch on.

In our case, you need to check the state of the file at the commit above.

I checked that and it seemed correct to me, line 122 refers to 
EXPORT_SYMBOL. So, I do not see the bug here.



Lukas

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
  2020-07-15  5:32                 ` Lukas Bulwahn
@ 2020-07-18  7:00                   ` Mrinal Pandey
  0 siblings, 0 replies; 13+ messages in thread
From: Mrinal Pandey @ 2020-07-18  7:00 UTC (permalink / raw)
  To: Lukas Bulwahn, Shuah Khan, Linux-kernel-mentees


[-- Attachment #1.1: Type: text/plain, Size: 3796 bytes --]

On Wed, Jul 15, 2020 at 11:02 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
wrote:

>
>
> On Tue, 14 Jul 2020, Mrinal Pandey wrote:
>
> >
> >
> > On Sat, Jul 11, 2020 at 3:17 AM Lukas Bulwahn <lukas.bulwahn@gmail.com>
> wrote:
> >
> > >> For Issue 5: Can you provide me (and the CC: the list) the list of
> > >> false positives (the commit hashes) you found for issue 5 on
> > >> EXPORT_SYMBOL?
> > >
> > >
> > > Here are the commit hashes for which the warning is issued:
> > > 54505a1e2083
> > > 75d75b7a4d54
> > > 8084c99b9af6
> > > bfdaf029c9c9
> > > dfd402a4c4ba
> > >
> > >> Can you also provide a short rationale/explanation for
> > >> each case that you considered a false positive?
> > >
> > >
> > > In each case the `EXPORT_SYMBOL()` is correctly written and the
> variable/function to be exported
> > > is also inside the parentheses, still, we get the warning. Please let
> me know if I am wrong here.
> >
> > I checked those warnings. Some of the patches are good cases to check if
> we can improve the heuristics on
> > EXPORT_SYMBOL().
> >
> > E.g., commit bfdaf029c9c9 ("ia64: turn csum_partial_copy_from_user()
> into csum_and_copy_from_user()") looks
> > sound to me, and checkpatch.pl should not really warn about that.
> >
> > Mrinal, maybe you can find out why checkpatch.pl believes that this
> case is wrong and needs to be warned
> > about?
> >
> >
> > Sir,
> >
> > Commit `54505a1e2083`, `8084c99b9af6` and `bfdaf029c9c9` have style
> issues. The documentation says to use
> > `EXPORT_SYMBOL()` just after the closing bracket `}` of the function, it
> is exporting. In these commits `EXPORT
> > SYMBOL()`
> > is either defined at a point later or has been defined after leaving a
> blank line which is totally unneeded.
> >
>
> Possibly, a new line is okay; I think we would some statistics on the
> overall use of EXPORT_SYMBOL() to see if a new line is generally accepted
> or if it is really a rare case (<3%) and checkpatch.pl should keep
> warning about that.
>

Sir,

I checked 2000 uses of EXPORT_SYMBOL, the total usages being 15800 approx.
and only 74 of them used a blank line.
That's 0.037% so probably checkpatch.pl is correct there. There are cases
where multiple exports are written together but these are also only
117 out of 2000.

>
> > However, commits `dfd402a4c4ba` and `75d75b7a4d54` look fine to me and
> have one thing in common, the
> > `EXPORT_SYMBOL()` in these is used inside of a macro. checkpatch
> shouldn't warn about these.
> >
>
> That is probably tricky to handle with checkpatch.pl, though.
>

I will let you know if I find something that works here. This is probably
the only case that is worth noticing.

>
> > I have noticed checkpatch denoting wrong line numbers for certain files
> for several commits.
> > For example, for the above commit
> > `bfdaf029c9c9`, the following warning is issued:
> >
> > WARNING: EXPORT_SYMBOL(foo); should immediately follow its
> function/variable
> > #74: FILE: arch/ia64/lib/csum_partial_copy.c:122:
> > +EXPORT_SYMBOL(csum_and_copy_from_user);
> >
> > but the file arch/ia64/lib/csum_partial_copy.c is only 113 lines long.
> > Please let me know if you can reproduce this.
>
> Can you find out how the line number is computed?
>
> Of course, checkpatch.pl is running on the patch, not on any specific
> file. So, in general, you really do not know how long the file was that
> the author based the patch on.
>
> In our case, you need to check the state of the file at the commit above.
>
> I checked that and it seemed correct to me, line 122 refers to
> EXPORT_SYMBOL. So, I do not see the bug here.
>
> Yes, the file is checked at that point in the commit and everything seems
to be correct. I was checking the
the current version of the file and that's why couldn't get line 122.

>
>
> Lukas
>

[-- Attachment #1.2: Type: text/html, Size: 5575 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-07-18  7:00 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-06  8:08 [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive Mrinal Pandey
2020-07-06 19:50 ` Lukas Bulwahn
     [not found]   ` <CAD1=X6m06KWb3=VEGkFWpmmV=UC09ZYbj2qpAOyg6OPFM=8KqA@mail.gmail.com>
     [not found]     ` <CAKXUXMy_KAHn+FNWviazVcEqK213uLVksq8_8HhjXAW+_OBrzQ@mail.gmail.com>
     [not found]       ` <CAD1=X6==MgT6jaNZ9PsPUqV1QTRFKguq9zvV+TtFSHyajFMjUg@mail.gmail.com>
2020-07-09  5:03         ` Lukas Bulwahn
2020-07-10 11:26           ` Mrinal Pandey
2020-07-10 20:46             ` Shuah Khan
2020-07-10 21:15               ` Lukas Bulwahn
2020-07-11 13:40                 ` Mrinal Pandey
2020-07-10 21:31             ` Lukas Bulwahn
2020-07-11 13:37               ` Mrinal Pandey
2020-07-10 21:47             ` Lukas Bulwahn
2020-07-14 14:56               ` Mrinal Pandey
2020-07-15  5:32                 ` Lukas Bulwahn
2020-07-18  7:00                   ` Mrinal Pandey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).