linux-kernel-mentees.lists.linuxfoundation.org archive mirror
 help / color / mirror / Atom feed
From: Mrinal Pandey <mrinalmni@gmail.com>
To: Lukas Bulwahn <lukas.bulwahn@gmail.com>,
	Shuah Khan <skhan@linuxfoundation.org>,
	 Linux-kernel-mentees@lists.linuxfoundation.org
Subject: Re: [Linux-kernel-mentees] [PATCH] checkpatch: Adjust spelling check false positive
Date: Sat, 11 Jul 2020 19:10:06 +0530	[thread overview]
Message-ID: <CAD1=X6mOEMz=uFh8yp4ouyYg+TRttPdQY=pfAyBnQ_m=Sh0OBw@mail.gmail.com> (raw)
In-Reply-To: <CAKXUXMyXnch8EHoEvm=1MbEREfpNv-r_QPTD6=h3HKWkQj5v=w@mail.gmail.com>


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

  reply	other threads:[~2020-07-11 13:40 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
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 [this message]
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='CAD1=X6mOEMz=uFh8yp4ouyYg+TRttPdQY=pfAyBnQ_m=Sh0OBw@mail.gmail.com' \
    --to=mrinalmni@gmail.com \
    --cc=Linux-kernel-mentees@lists.linuxfoundation.org \
    --cc=lukas.bulwahn@gmail.com \
    --cc=skhan@linuxfoundation.org \
    --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).