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, 18 Jul 2020 12:30:30 +0530 [thread overview]
Message-ID: <CAD1=X6kv+PxEu8CBaEHORyVcgQz33obgheJ8FtTu30vG6VOaGQ@mail.gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2007150706410.13346@felia>
[-- 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
prev parent reply other threads:[~2020-07-18 7:00 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
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=X6kv+PxEu8CBaEHORyVcgQz33obgheJ8FtTu30vG6VOaGQ@mail.gmail.com' \
--to=mrinalmni@gmail.com \
--cc=Linux-kernel-mentees@lists.linuxfoundation.org \
--cc=lukas.bulwahn@gmail.com \
--cc=skhan@linuxfoundation.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).