From: Lukas Bulwahn <firstname.lastname@example.org> To: Mrinal Pandey <email@example.com> Cc: Linuxfirstname.lastname@example.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 <email@example.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 Linuxfirstname.lastname@example.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next prev parent 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 \ --email@example.com \ --cc=Linuxfirstname.lastname@example.org \ --email@example.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).