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: Wed, 15 Jul 2020 07:32:25 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.21.2007150706410.13346@felia> (raw)
In-Reply-To: <CAD1=X6m2Lp-KE-xHN1uE7egZRp5-gkXbGVC4f5EUjyMfJ12rHQ@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]



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



Lukas

[-- 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-15  5:32 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
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 [this message]
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=alpine.DEB.2.21.2007150706410.13346@felia \
    --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).