[Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive

Lukas Bulwahn lukas.bulwahn at gmail.com
Fri Jul 10 21:15:57 UTC 2020


On Fri, Jul 10, 2020 at 10:46 PM Shuah Khan <skhan at 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 at gmail.com
> > <mailto:lukas.bulwahn at gmail.com>> wrote:
> >
> >     On Thu, Jul 9, 2020 at 6:31 AM Mrinal Pandey <mrinalmni at gmail.com
> >     <mailto:mrinalmni at gmail.com>> wrote:
> >      >
> >      > On Tue, Jul 7, 2020 at 10:24 PM Lukas Bulwahn
> >     <lukas.bulwahn at gmail.com <mailto:lukas.bulwahn at gmail.com>> wrote:
> >      >>
> >      >> On Tue, Jul 7, 2020 at 10:22 AM Mrinal Pandey
> >     <mrinalmni at gmail.com <mailto:mrinalmni at gmail.com>> wrote:
> >      >> >
> >      >> >
> >      >> >
> >      >> > On Tue, Jul 7, 2020 at 1:20 AM Lukas Bulwahn
> >     <lukas.bulwahn at gmail.com <mailto:lukas.bulwahn at gmail.com>> wrote:
> >      >> >>
> >      >> >> On Mon, Jul 6, 2020 at 10:08 AM Mrinal Pandey
> >     <mrinalmni at gmail.com <mailto:mrinalmni at 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 at gmail.com
> >     <mailto:mrinalmni at 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 at lists.linuxfoundation.org
> >     <mailto:linux-kernel-mentees at lists.linuxfoundation.org> back to the
> >     recipient list.)
> >     Also, on sending emails: you started the thread on
> >     linux-kernel-mentees at lists.linuxfoundation.org
> >     <mailto:linux-kernel-mentees at 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 at canonical.com> (maintainer:CHECKPATCH)
Joe Perches <joe at perches.com> (maintainer:CHECKPATCH)
linux-kernel at vger.kernel.org (open list)

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

Lukas


More information about the Linux-kernel-mentees mailing list