Starting commented lines in a file mostly contains comments describing
license, copyright or general information about the file.
E.g., in sound/pci/ctxfi/ctresource.c, initial comment lines describe
its copyright and other related file informations.
But as kernel-doc reads these lines, it results in ineffective warnings by
kernel-doc, related to these.
Provide a simple fix by skipping first three lines in a file for checking
kernel-doc comments.
Suggested-by: Lukas Bulwahn <[email protected]>
Signed-off-by: Aditya Srivastava <[email protected]>
---
scripts/kernel-doc | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index e1e562b2e2e7..431add05248e 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -2375,6 +2375,7 @@ sub process_file($) {
my $file;
my $initial_section_counter = $section_counter;
my ($orig_file) = @_;
+ my $lineno = 0; # to maintain the count of line number in a file
$file = map_filename($orig_file);
@@ -2388,13 +2389,16 @@ sub process_file($) {
$section_counter = 0;
while (<IN_FILE>) {
+ $lineno++;
while (s/\\\s*$//) {
$_ .= <IN_FILE>;
+ $lineno++;
}
# Replace tabs by spaces
while ($_ =~ s/\t+/' ' x (length($&) * 8 - length($`) % 8)/e) {};
# Hand this line to the appropriate state handler
- if ($state == STATE_NORMAL) {
+ if ($state == STATE_NORMAL
+ && $lineno > 3) { # to avoid starting comment lines describing the file
process_normal();
} elsif ($state == STATE_NAME) {
process_name($file, $_);
--
2.17.1
Am 09.03.21 um 13:53 schrieb Aditya Srivastava:
> Starting commented lines in a file mostly contains comments describing
> license, copyright or general information about the file.
>
> E.g., in sound/pci/ctxfi/ctresource.c, initial comment lines describe
> its copyright and other related file informations.
The opening comment mark /** is used for kernel-doc comments [1]
[1]
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments
-- Markus --
>
> But as kernel-doc reads these lines, it results in ineffective warnings by
> kernel-doc, related to these.
>
> Provide a simple fix by skipping first three lines in a file for checking
> kernel-doc comments.
>
> Suggested-by: Lukas Bulwahn <[email protected]>
> Signed-off-by: Aditya Srivastava <[email protected]>
> ---
> scripts/kernel-doc | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/kernel-doc b/scripts/kernel-doc
> index e1e562b2e2e7..431add05248e 100755
> --- a/scripts/kernel-doc
> +++ b/scripts/kernel-doc
> @@ -2375,6 +2375,7 @@ sub process_file($) {
> my $file;
> my $initial_section_counter = $section_counter;
> my ($orig_file) = @_;
> + my $lineno = 0; # to maintain the count of line number in a file
>
> $file = map_filename($orig_file);
>
> @@ -2388,13 +2389,16 @@ sub process_file($) {
>
> $section_counter = 0;
> while (<IN_FILE>) {
> + $lineno++;
> while (s/\\\s*$//) {
> $_ .= <IN_FILE>;
> + $lineno++;
> }
> # Replace tabs by spaces
> while ($_ =~ s/\t+/' ' x (length($&) * 8 - length($`) % 8)/e) {};
> # Hand this line to the appropriate state handler
> - if ($state == STATE_NORMAL) {
> + if ($state == STATE_NORMAL
> + && $lineno > 3) { # to avoid starting comment lines describing the file
> process_normal();
> } elsif ($state == STATE_NAME) {
> process_name($file, $_);
>
On 9/3/21 7:00 pm, Markus Heiser wrote:
>
> Am 09.03.21 um 13:53 schrieb Aditya Srivastava:
>> Starting commented lines in a file mostly contains comments describing
>> license, copyright or general information about the file.
>>
>> E.g., in sound/pci/ctxfi/ctresource.c, initial comment lines describe
>> its copyright and other related file informations.
>
> The opening comment mark /** is used for kernel-doc comments [1]
>
> [1]
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments
>
Hi Markus!
That's true. But the content inside the comment does not follow
kernel-doc format.
For e.g., try running kernel-doc -none/man/rst on the above file in
the example("sound/pci/ctxfi/ctresource.c").
The starting 2-3 lines in files generally do not contain any
struct/enum/function, etc. declaration.
Thanks
Aditya
On Tue, Mar 9, 2021 at 10:24 PM Aditya <[email protected]> wrote:
>
> On 9/3/21 7:00 pm, Markus Heiser wrote:
> >
> > Am 09.03.21 um 13:53 schrieb Aditya Srivastava:
> >> Starting commented lines in a file mostly contains comments describing
> >> license, copyright or general information about the file.
> >>
> >> E.g., in sound/pci/ctxfi/ctresource.c, initial comment lines describe
> >> its copyright and other related file informations.
> >
> > The opening comment mark /** is used for kernel-doc comments [1]
> >
> > [1]
> > https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments
> >
>
> Hi Markus!
> That's true. But the content inside the comment does not follow
> kernel-doc format.
> For e.g., try running kernel-doc -none/man/rst on the above file in
> the example("sound/pci/ctxfi/ctresource.c").
> The starting 2-3 lines in files generally do not contain any
> struct/enum/function, etc. declaration.
>
Aditya, can you provide a diff of the warnings over the whole kernel tree?
At the moment, your patch just implements ignoring the initial
comment, which probably is good for experimentation.
Alternatively, we could simply have a dedicated warning and then
ignore it or even warn and then parse it as-if.
In the "long run", we would probably want to fix all current files in
the repository by just replacing '/**' by '/*' and have kernel-doc
warn about this suspicious pattern, when new files appear (maybe even
configurable, but that is another feature to enable or disable certain
kernel-doc checks and warnings). I would certainly assist and
contribute to such a clean-up task.
I think the first step is to look at the diff, and see how many cases
really appear in the tree... then check how many patches throughout
the whole tree are required and if they are generally accepted.
Lukas
On 10/3/21 11:49 am, Lukas Bulwahn wrote:
> On Tue, Mar 9, 2021 at 10:24 PM Aditya <[email protected]> wrote:
>>
>> On 9/3/21 7:00 pm, Markus Heiser wrote:
>>>
>>> Am 09.03.21 um 13:53 schrieb Aditya Srivastava:
>>>> Starting commented lines in a file mostly contains comments describing
>>>> license, copyright or general information about the file.
>>>>
>>>> E.g., in sound/pci/ctxfi/ctresource.c, initial comment lines describe
>>>> its copyright and other related file informations.
>>>
>>> The opening comment mark /** is used for kernel-doc comments [1]
>>>
>>> [1]
>>> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments
>>>
>>
>> Hi Markus!
>> That's true. But the content inside the comment does not follow
>> kernel-doc format.
>> For e.g., try running kernel-doc -none/man/rst on the above file in
>> the example("sound/pci/ctxfi/ctresource.c").
>> The starting 2-3 lines in files generally do not contain any
>> struct/enum/function, etc. declaration.
>>
>
> Aditya, can you provide a diff of the warnings over the whole kernel tree?
>
> At the moment, your patch just implements ignoring the initial
> comment, which probably is good for experimentation.
>
> Alternatively, we could simply have a dedicated warning and then
> ignore it or even warn and then parse it as-if.
>
> In the "long run", we would probably want to fix all current files in
> the repository by just replacing '/**' by '/*' and have kernel-doc
> warn about this suspicious pattern, when new files appear (maybe even
> configurable, but that is another feature to enable or disable certain
> kernel-doc checks and warnings). I would certainly assist and
> contribute to such a clean-up task.
>
> I think the first step is to look at the diff, and see how many cases
> really appear in the tree... then check how many patches throughout
> the whole tree are required and if they are generally accepted.
>
Hi Lukas!
This is the diff of the warnings over kernel tree before and after
applying these changes.
There are 2 sections in this report:
1) for the warnings present before, but not after, and;
2) after but not before
The part (2) contains, for some cases, where the warning for "warning:
Incorrect use of kernel-doc format:" type has changed to "warning:
wrong kernel-doc identifier on line:" type.
The diff file can be found at:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/kernel-doc/avoid_init_line_diff.txt
Thanks
Aditya
On Thu, Mar 11, 2021 at 10:04 PM Aditya <[email protected]> wrote:
>
> On 10/3/21 11:49 am, Lukas Bulwahn wrote:
> > On Tue, Mar 9, 2021 at 10:24 PM Aditya <[email protected]> wrote:
> >>
> >> On 9/3/21 7:00 pm, Markus Heiser wrote:
> >>>
> >>> Am 09.03.21 um 13:53 schrieb Aditya Srivastava:
> >>>> Starting commented lines in a file mostly contains comments describing
> >>>> license, copyright or general information about the file.
> >>>>
> >>>> E.g., in sound/pci/ctxfi/ctresource.c, initial comment lines describe
> >>>> its copyright and other related file informations.
> >>>
> >>> The opening comment mark /** is used for kernel-doc comments [1]
> >>>
> >>> [1]
> >>> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments
> >>>
> >>
> >> Hi Markus!
> >> That's true. But the content inside the comment does not follow
> >> kernel-doc format.
> >> For e.g., try running kernel-doc -none/man/rst on the above file in
> >> the example("sound/pci/ctxfi/ctresource.c").
> >> The starting 2-3 lines in files generally do not contain any
> >> struct/enum/function, etc. declaration.
> >>
> >
> > Aditya, can you provide a diff of the warnings over the whole kernel tree?
> >
> > At the moment, your patch just implements ignoring the initial
> > comment, which probably is good for experimentation.
> >
> > Alternatively, we could simply have a dedicated warning and then
> > ignore it or even warn and then parse it as-if.
> >
> > In the "long run", we would probably want to fix all current files in
> > the repository by just replacing '/**' by '/*' and have kernel-doc
> > warn about this suspicious pattern, when new files appear (maybe even
> > configurable, but that is another feature to enable or disable certain
> > kernel-doc checks and warnings). I would certainly assist and
> > contribute to such a clean-up task.
> >
> > I think the first step is to look at the diff, and see how many cases
> > really appear in the tree... then check how many patches throughout
> > the whole tree are required and if they are generally accepted.
> >
>
> Hi Lukas!
> This is the diff of the warnings over kernel tree before and after
> applying these changes.
> There are 2 sections in this report:
> 1) for the warnings present before, but not after, and;
> 2) after but not before
>
> The part (2) contains, for some cases, where the warning for "warning:
> Incorrect use of kernel-doc format:" type has changed to "warning:
> wrong kernel-doc identifier on line:" type.
>
> The diff file can be found at:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/kernel-doc/avoid_init_line_diff.txt
>
Thanks, let us check if we can use this diff to create a patch set
that cleans up those header comments for those files.
Lukas
Aditya <[email protected]> writes:
>> The opening comment mark /** is used for kernel-doc comments [1]
>>
>> [1]
>> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments
>>
>
> Hi Markus!
> That's true. But the content inside the comment does not follow
> kernel-doc format.
> For e.g., try running kernel-doc -none/man/rst on the above file in
> the example("sound/pci/ctxfi/ctresource.c").
> The starting 2-3 lines in files generally do not contain any
> struct/enum/function, etc. declaration.
The problem is that it's marked as a kerneldoc comment without actually
being one; it looks like somebody's internal corporate formatting. The
fix is not to put a hack into kernel-doc - we have more than enough of
those in the file already! The right thing to do is to remove the extra
"*" so that the comment doesn't look like a kerneldoc comment anymore.
Thanks,
jon
On Mon, Mar 15, 2021 at 8:25 PM Jonathan Corbet <[email protected]> wrote:
>
> Aditya <[email protected]> writes:
>
> >> The opening comment mark /** is used for kernel-doc comments [1]
> >>
> >> [1]
> >> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#how-to-format-kernel-doc-comments
> >>
> >
> > Hi Markus!
> > That's true. But the content inside the comment does not follow
> > kernel-doc format.
> > For e.g., try running kernel-doc -none/man/rst on the above file in
> > the example("sound/pci/ctxfi/ctresource.c").
> > The starting 2-3 lines in files generally do not contain any
> > struct/enum/function, etc. declaration.
>
> The problem is that it's marked as a kerneldoc comment without actually
> being one; it looks like somebody's internal corporate formatting. The
> fix is not to put a hack into kernel-doc - we have more than enough of
> those in the file already! The right thing to do is to remove the extra
> "*" so that the comment doesn't look like a kerneldoc comment anymore.
>
Jonathan, I agree that that is the right thing to do. Aditya is
already following through and is cleaning up the repository. So, let
us be optimistic that we will have cleaned up all of those occurrences
within a few weeks. But how to continue? Someone is going to come with
new files and introduce this pattern again in the repository; and as
of now, we do not have a script to identify that pattern and react...
Running kernel-doc on the whole tree continuously and just observing
the new warnings is probably not going to work as of now: there are
20,000 kernel-doc warnings and at least, I cannot see a really good
way to filter out this one specific type of issue among all the
warnings that will might appear in the future (without specifically
applying Aditya's patch and looking at the diff before and after).
I wonder if we could extend kernel-doc (not your preferred option as
it seems) for a new dedicated warning message or maintain a separate
kernel-doc sanity checking script to emit a dedicated warning based on
some heuristics that suggests when a "header comment" is probably
unintentionally declared as a "kernel-doc comment" when it really
should not be.
Jonathan, would you then prefer to have a separate kernel-doc sanity
checking script that then allows us to maintain checking for patterns
we already cleaned up?
Eventually, we might have cleaned up enough to just use kernel-doc and
keep it kind of warning-free (as with make htmldocs now) and then, we
can simply follow up with kernel-doc and some basic monitoring
scripts, but with 20,000 warnings in the whole repository to start
with, it is still a long way to that point, IMHO.
Lukas
Lukas Bulwahn <[email protected]> writes:
> I wonder if we could extend kernel-doc (not your preferred option as
> it seems) for a new dedicated warning message or maintain a separate
> kernel-doc sanity checking script to emit a dedicated warning based on
> some heuristics that suggests when a "header comment" is probably
> unintentionally declared as a "kernel-doc comment" when it really
> should not be.
>
> Jonathan, would you then prefer to have a separate kernel-doc sanity
> checking script that then allows us to maintain checking for patterns
> we already cleaned up?
Having a warning in kernel-doc for "This comment starts with /** but
isn't a kerneldoc comment" could be useful, I guess. That is the real
problem, not the fact that it appears at the top of the file. I'm all
for tools that help us to clean things up, but let's not add
line-counting hacks to try to paper it over.
Thanks,
jon
On Thu, Mar 18, 2021 at 5:37 PM Jonathan Corbet <[email protected]> wrote:
>
> Lukas Bulwahn <[email protected]> writes:
>
> > I wonder if we could extend kernel-doc (not your preferred option as
> > it seems) for a new dedicated warning message or maintain a separate
> > kernel-doc sanity checking script to emit a dedicated warning based on
> > some heuristics that suggests when a "header comment" is probably
> > unintentionally declared as a "kernel-doc comment" when it really
> > should not be.
> >
> > Jonathan, would you then prefer to have a separate kernel-doc sanity
> > checking script that then allows us to maintain checking for patterns
> > we already cleaned up?
>
> Having a warning in kernel-doc for "This comment starts with /** but
> isn't a kerneldoc comment" could be useful, I guess. That is the real
> problem, not the fact that it appears at the top of the file. I'm all
> for tools that help us to clean things up, but let's not add
> line-counting hacks to try to paper it over.
>
Yeah, and as this line-counting is really just a poor man's
heuristics, we might just be better to really turn this heuristics
into a dedicated cleanup warning script, then we can check for more
indicators, such as "does it contain the word Copyright" somewhere in
the kernel-doc comment, which tells us even more that this is not a
kernel-doc as we would expect it.
Aditya, would you like to try to turn this check into a separate script instead?
Lukas
Lukas Bulwahn <[email protected]> writes:
> Yeah, and as this line-counting is really just a poor man's
> heuristics, we might just be better to really turn this heuristics
> into a dedicated cleanup warning script, then we can check for more
> indicators, such as "does it contain the word Copyright" somewhere in
> the kernel-doc comment, which tells us even more that this is not a
> kernel-doc as we would expect it.
I really don't think we need that kind of heuristic. The format of
kerneldoc comments is fairly rigid; it shouldn't be too hard to pick out
the /** comments that don't fit that format, right? Am I missing
something there?
Thanks,
jon
On 18/3/21 11:48 pm, Jonathan Corbet wrote:
> Lukas Bulwahn <[email protected]> writes:
>
>> Yeah, and as this line-counting is really just a poor man's
>> heuristics, we might just be better to really turn this heuristics
>> into a dedicated cleanup warning script, then we can check for more
>> indicators, such as "does it contain the word Copyright" somewhere in
>> the kernel-doc comment, which tells us even more that this is not a
>> kernel-doc as we would expect it.
>
> I really don't think we need that kind of heuristic. The format of
> kerneldoc comments is fairly rigid; it shouldn't be too hard to pick out
> the /** comments that don't fit that format, right? Am I missing
> something there?
>
> Thanks,
>
> jon
>
Thanks for the inputs Lukas and Jonathan. I shall try to come up with
something.
Thanks
Aditya
On 20/3/21 12:23 pm, Aditya wrote:
> On 18/3/21 11:48 pm, Jonathan Corbet wrote:
>> Lukas Bulwahn <[email protected]> writes:
>>
>>> Yeah, and as this line-counting is really just a poor man's
>>> heuristics, we might just be better to really turn this heuristics
>>> into a dedicated cleanup warning script, then we can check for more
>>> indicators, such as "does it contain the word Copyright" somewhere in
>>> the kernel-doc comment, which tells us even more that this is not a
>>> kernel-doc as we would expect it.
>>
>> I really don't think we need that kind of heuristic. The format of
>> kerneldoc comments is fairly rigid; it shouldn't be too hard to pick out
>> the /** comments that don't fit that format, right? Am I missing
>> something there?
>>
>> Thanks,
>>
>> jon
>>
Hi Lukas and Jon!
I have a question, should I clean up the files with '/**' like
comments in only header lines? Or as we are planning for making it
generic, for other lines as well?
Thanks
Aditya
On Sat, Mar 20, 2021 at 1:45 PM Aditya Srivastava <[email protected]> wrote:
>
> On 20/3/21 12:23 pm, Aditya wrote:
> > On 18/3/21 11:48 pm, Jonathan Corbet wrote:
> >> Lukas Bulwahn <[email protected]> writes:
> >>
> >>> Yeah, and as this line-counting is really just a poor man's
> >>> heuristics, we might just be better to really turn this heuristics
> >>> into a dedicated cleanup warning script, then we can check for more
> >>> indicators, such as "does it contain the word Copyright" somewhere in
> >>> the kernel-doc comment, which tells us even more that this is not a
> >>> kernel-doc as we would expect it.
> >>
> >> I really don't think we need that kind of heuristic. The format of
> >> kerneldoc comments is fairly rigid; it shouldn't be too hard to pick out
> >> the /** comments that don't fit that format, right? Am I missing
> >> something there?
> >>
> >> Thanks,
> >>
> >> jon
> >>
>
> Hi Lukas and Jon!
> I have a question, should I clean up the files with '/**' like
> comments in only header lines? Or as we are planning for making it
> generic, for other lines as well?
>
Aditya, of course, if you can detect and come across some unintended
'/**' comments in some files, clean them in the same go (as you did
with ecryptfs).
I am just worried that if you extend it to the fully generic case,
that the list of cases simply explodes: showing many 1,000 cases
across various 1,000 files that need to be cleaned up, and such
clean-up work is just too much to get done by yourself.
The current list limited to comments in header lines seems to be a set
of patches that you can probably get done.
Lukas
On 20/3/21 6:51 pm, Lukas Bulwahn wrote:
> On Sat, Mar 20, 2021 at 1:45 PM Aditya Srivastava <[email protected]> wrote:
>>
>> On 20/3/21 12:23 pm, Aditya wrote:
>>> On 18/3/21 11:48 pm, Jonathan Corbet wrote:
>>>> Lukas Bulwahn <[email protected]> writes:
>>>>
>>>>> Yeah, and as this line-counting is really just a poor man's
>>>>> heuristics, we might just be better to really turn this heuristics
>>>>> into a dedicated cleanup warning script, then we can check for more
>>>>> indicators, such as "does it contain the word Copyright" somewhere in
>>>>> the kernel-doc comment, which tells us even more that this is not a
>>>>> kernel-doc as we would expect it.
>>>>
>>>> I really don't think we need that kind of heuristic. The format of
>>>> kerneldoc comments is fairly rigid; it shouldn't be too hard to pick out
>>>> the /** comments that don't fit that format, right? Am I missing
>>>> something there?
>>>>
>>>> Thanks,
>>>>
>>>> jon
>>>>
>>
>> Hi Lukas and Jon!
>> I have a question, should I clean up the files with '/**' like
>> comments in only header lines? Or as we are planning for making it
>> generic, for other lines as well?
>>
>
> Aditya, of course, if you can detect and come across some unintended
> '/**' comments in some files, clean them in the same go (as you did
> with ecryptfs).
>
> I am just worried that if you extend it to the fully generic case,
> that the list of cases simply explodes: showing many 1,000 cases
> across various 1,000 files that need to be cleaned up, and such
> clean-up work is just too much to get done by yourself.
>
> The current list limited to comments in header lines seems to be a set
> of patches that you can probably get done.
>
Sounds good, Lukas.
Thanks
Aditya