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
>