* [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
[parent not found: <CAD1=X6m06KWb3=VEGkFWpmmV=UC09ZYbj2qpAOyg6OPFM=8KqA@mail.gmail.com>]
[parent not found: <CAKXUXMy_KAHn+FNWviazVcEqK213uLVksq8_8HhjXAW+_OBrzQ@mail.gmail.com>]
[parent not found: <CAD1=X6==MgT6jaNZ9PsPUqV1QTRFKguq9zvV+TtFSHyajFMjUg@mail.gmail.com>]
* 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 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 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 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 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: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).