linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
From: Lukas Bulwahn <lukas.bulwahn@gmail.com>
To: Mrinal Pandey <mrinalmni@gmail.com>
Cc: Linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
Date: Thu, 9 Jul 2020 07:03:00 +0200	[thread overview]
Message-ID: <CAKXUXMwU21sUTA6vSm19oFF42eX3dxuBEG-cgSoUQBWcYJDOiQ@mail.gmail.com> (raw)
In-Reply-To: <CAD1=X6==MgT6jaNZ9PsPUqV1QTRFKguq9zvV+TtFSHyajFMjUg@mail.gmail.com>

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

  parent reply	other threads:[~2020-07-09  5:03 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-06  8:08 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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKXUXMwU21sUTA6vSm19oFF42eX3dxuBEG-cgSoUQBWcYJDOiQ@mail.gmail.com \
    --to=lukas.bulwahn@gmail.com \
    --cc=Linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=mrinalmni@gmail.com \
    --subject='Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).