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

[Mrinal Pandey <mrinalmni@gmail.com>]

[-- 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