anonymous enums in kernel doc

anonymous enums in kernel doc

[Andy Shevchenko]

Mauro, can you do some test cases in your workflow against anonymous
enum in ernel doc, please?

They are broken again, please fix the script!

drivers/pinctrl/intel/pinctrl-intel.c:204: warning: wrong kernel-doc
identifier on line:
* enum - Locking variants of the pad configuration

Above is simply a wrong statement.

-- 
With Best Regards,
Andy Shevchenko

Re: anonymous enums in kernel doc

[Jonathan Corbet]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> Mauro, can you do some test cases in your workflow against anonymous
> enum in ernel doc, please?
>
> They are broken again, please fix the script!
>
> drivers/pinctrl/intel/pinctrl-intel.c:204: warning: wrong kernel-doc
> identifier on line:
> * enum - Locking variants of the pad configuration
>
> Above is simply a wrong statement.

The real problem, perhaps, is that there seems to be little point in
adding kerneldoc comments for anonymous enums; where are you going to
use that documentation?  The error message could perhaps be changed to
say that; meanwhile, perhaps this one could be fixed with an action like
s%/**%/*% ?

Thanks,

jon

Re: anonymous enums in kernel doc

[Andy Shevchenko]

On Tue, Feb 16, 2021 at 6:51 PM Jonathan Corbet <corbet@lwn.net> wrote:
>
> > Mauro, can you do some test cases in your workflow against anonymous
> > enum in ernel doc, please?
> >
> > They are broken again, please fix the script!
> >
> > drivers/pinctrl/intel/pinctrl-intel.c:204: warning: wrong kernel-doc
> > identifier on line:
> > * enum - Locking variants of the pad configuration
> >
> > Above is simply a wrong statement.
>
> The real problem, perhaps, is that there seems to be little point in
> adding kerneldoc comments for anonymous enums; where are you going to
> use that documentation?

I had been explicitly told during review (IIRC by maintainers) to make
it such, while the initial version was exactly like you are thinking
of. So, I'm not the right person to be asked :-)

>  The error message could perhaps be changed to
> say that; meanwhile, perhaps this one could be fixed with an action like
> s%/**%/*% ?

See above. I think regression comes from the kernel doc script,
earlier it was okay. That said, the author of kernel doc changes has
to submit a patch to amend the driver and maintainers will review it.

-- 
With Best Regards,
Andy Shevchenko

Re: anonymous enums in kernel doc

[Jonathan Corbet]

Andy Shevchenko <andy.shevchenko@gmail.com> writes:

> On Tue, Feb 16, 2021 at 6:51 PM Jonathan Corbet <corbet@lwn.net> wrote:
>>
>> > Mauro, can you do some test cases in your workflow against anonymous
>> > enum in ernel doc, please?
>> >
>> > They are broken again, please fix the script!
>> >
>> > drivers/pinctrl/intel/pinctrl-intel.c:204: warning: wrong kernel-doc
>> > identifier on line:
>> > * enum - Locking variants of the pad configuration
>> >
>> > Above is simply a wrong statement.
>>
>> The real problem, perhaps, is that there seems to be little point in
>> adding kerneldoc comments for anonymous enums; where are you going to
>> use that documentation?
>
> I had been explicitly told during review (IIRC by maintainers) to make
> it such, while the initial version was exactly like you are thinking
> of. So, I'm not the right person to be asked :-)
>
>>  The error message could perhaps be changed to
>> say that; meanwhile, perhaps this one could be fixed with an action like
>> s%/**%/*% ?
>
> See above. I think regression comes from the kernel doc script,
> earlier it was okay. That said, the author of kernel doc changes has
> to submit a patch to amend the driver and maintainers will review it.

kerneldoc now warns about various incorrect things that it used to just
silently pass over.  There is no regression here, just a new diagnostic
to point out something that was never going to work right.  Unless you
have a good idea for what kerneldoc should do with a block like that?

(An alternative fix, of course, would be to give the enum a name so it
can actually be used for type checking.)

Thanks,

jon

Re: anonymous enums in kernel doc

[Andy Shevchenko]

On Tue, Feb 16, 2021 at 7:05 PM Jonathan Corbet <corbet@lwn.net> wrote:
>
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
>
> > On Tue, Feb 16, 2021 at 6:51 PM Jonathan Corbet <corbet@lwn.net> wrote:
> >>
> >> > Mauro, can you do some test cases in your workflow against anonymous
> >> > enum in ernel doc, please?
> >> >
> >> > They are broken again, please fix the script!
> >> >
> >> > drivers/pinctrl/intel/pinctrl-intel.c:204: warning: wrong kernel-doc
> >> > identifier on line:
> >> > * enum - Locking variants of the pad configuration
> >> >
> >> > Above is simply a wrong statement.
> >>
> >> The real problem, perhaps, is that there seems to be little point in
> >> adding kerneldoc comments for anonymous enums; where are you going to
> >> use that documentation?
> >
> > I had been explicitly told during review (IIRC by maintainers) to make
> > it such, while the initial version was exactly like you are thinking
> > of. So, I'm not the right person to be asked :-)

Just for a reference [1].

> >>  The error message could perhaps be changed to
> >> say that; meanwhile, perhaps this one could be fixed with an action like
> >> s%/**%/*% ?
> >
> > See above. I think regression comes from the kernel doc script,
> > earlier it was okay. That said, the author of kernel doc changes has
> > to submit a patch to amend the driver and maintainers will review it.
>
> kerneldoc now warns about various incorrect things that it used to just
> silently pass over.  There is no regression here, just a new diagnostic
> to point out something that was never going to work right.  Unless you
> have a good idea for what kerneldoc should do with a block like that?

As it does, put description of individual fields and prepend it with a
common part.

So,

enum - Bla bla bla
 @FOO: ABC
 @BAR: DEF
Description

Should go in the doc for the corresponding file like (as an example)

Anonymous enumeration Bla bla bla
Description

FOO ABC
BAR DEF

(not sure about indentation, emphasizing and separators, but I think
you got the idea).

> (An alternative fix, of course, would be to give the enum a name so it
> can actually be used for type checking.)

That enum is not used as an enum, it provides the logically unified constants.

Personally I don't see why the kernel doc can't digest this.

[1]: https://patchwork.ozlabs.org/project/linux-gpio/patch/20190808132128.13359-1-andriy.shevchenko@linux.intel.com/

-- 
With Best Regards,
Andy Shevchenko

[PATCH] scripts/kernel-doc: ignore identifier on anonymous enums

[Mauro Carvalho Chehab]

When anonymous enums are used, the identifier is empty.

While, IMO, it should be avoided the usage of such enums,
adding support for it is not hard.

So, postpone the check for empty identifiers to happen
only at the dump phase.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 scripts/kernel-doc | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/scripts/kernel-doc b/scripts/kernel-doc
index 68df17877384..f8ebbf5e9f12 100755
--- a/scripts/kernel-doc
+++ b/scripts/kernel-doc
@@ -1412,9 +1412,14 @@ sub dump_enum($$) {
 
     if ($members) {
 	if ($identifier ne $declaration_name) {
-	    print STDERR "${file}:$.: warning: expecting prototype for enum $identifier. Prototype was for enum $declaration_name instead\n";
+	    if ($identifier eq "") {
+		print STDERR "${file}:$.: warning: wrong kernel-doc identifier on line:\n";
+	    } else {
+		print STDERR "${file}:$.: warning: expecting prototype for enum $identifier. Prototype was for enum $declaration_name instead\n";
+	    }
 	    return;
 	}
+	$declaration_name = "(anonymous)" if ($declaration_name eq "");
 
 	my %_members;
 
@@ -2132,7 +2137,7 @@ sub process_name($$) {
 	    ++$warnings;
 	}
 
-	if ($identifier eq "") {
+	if ($identifier eq "" && $decl_type ne "enum") {
 	    print STDERR "${file}:$.: warning: wrong kernel-doc identifier on line:\n";
 	    print STDERR $_;
 	    ++$warnings;
-- 
2.29.2

Re: anonymous enums in kernel doc

[Mauro Carvalho Chehab]

Em Tue, 16 Feb 2021 19:12:58 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:

> On Tue, Feb 16, 2021 at 7:05 PM Jonathan Corbet <corbet@lwn.net> wrote:
> >
> > Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> >  
> > > On Tue, Feb 16, 2021 at 6:51 PM Jonathan Corbet <corbet@lwn.net> wrote:  
> > >>  
> > >> > Mauro, can you do some test cases in your workflow against anonymous
> > >> > enum in ernel doc, please?
> > >> >
> > >> > They are broken again, please fix the script!
> > >> >
> > >> > drivers/pinctrl/intel/pinctrl-intel.c:204: warning: wrong kernel-doc
> > >> > identifier on line:
> > >> > * enum - Locking variants of the pad configuration
> > >> >
> > >> > Above is simply a wrong statement.  
> > >>
> > >> The real problem, perhaps, is that there seems to be little point in
> > >> adding kerneldoc comments for anonymous enums; where are you going to
> > >> use that documentation?  
> > >
> > > I had been explicitly told during review (IIRC by maintainers) to make
> > > it such, while the initial version was exactly like you are thinking
> > > of. So, I'm not the right person to be asked :-)  
> 
> Just for a reference [1].
> 
> > >>  The error message could perhaps be changed to
> > >> say that; meanwhile, perhaps this one could be fixed with an action like
> > >> s%/**%/*% ?  
> > >
> > > See above. I think regression comes from the kernel doc script,
> > > earlier it was okay. That said, the author of kernel doc changes has
> > > to submit a patch to amend the driver and maintainers will review it.  
> >
> > kerneldoc now warns about various incorrect things that it used to just
> > silently pass over.  There is no regression here, just a new diagnostic
> > to point out something that was never going to work right.  Unless you
> > have a good idea for what kerneldoc should do with a block like that?  
> 
> As it does, put description of individual fields and prepend it with a
> common part.
> 
> So,
> 
> enum - Bla bla bla
>  @FOO: ABC
>  @BAR: DEF
> Description
> 
> Should go in the doc for the corresponding file like (as an example)
> 
> Anonymous enumeration Bla bla bla
> Description
> 
> FOO ABC
> BAR DEF
> 
> (not sure about indentation, emphasizing and separators, but I think
> you got the idea).
> 
> > (An alternative fix, of course, would be to give the enum a name so it
> > can actually be used for type checking.)  
> 
> That enum is not used as an enum, it provides the logically unified constants.

What's the problem of giving it a name?

You could call it as "intel_pinctrl_pad" or something similar.

> Personally I don't see why the kernel doc can't digest this.

It is not hard to add support for this special case. Just sent a
patch.

Yet, this adds additional complexity on an script that it is
already complex enough.

> 
> [1]: https://patchwork.ozlabs.org/project/linux-gpio/patch/20190808132128.13359-1-andriy.shevchenko@linux.intel.com/
> 

Thanks,
Mauro

Re: anonymous enums in kernel doc

[Andy Shevchenko]

On Wed, Mar 3, 2021 at 10:44 AM Mauro Carvalho Chehab
<mchehab@kernel.org> wrote:
> Em Tue, 16 Feb 2021 19:12:58 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:
> > On Tue, Feb 16, 2021 at 7:05 PM Jonathan Corbet <corbet@lwn.net> wrote:
> > > Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> > > > On Tue, Feb 16, 2021 at 6:51 PM Jonathan Corbet <corbet@lwn.net> wrote:
> > > >>
> > > >> > Mauro, can you do some test cases in your workflow against anonymous
> > > >> > enum in ernel doc, please?
> > > >> >
> > > >> > They are broken again, please fix the script!
> > > >> >
> > > >> > drivers/pinctrl/intel/pinctrl-intel.c:204: warning: wrong kernel-doc
> > > >> > identifier on line:
> > > >> > * enum - Locking variants of the pad configuration
> > > >> >
> > > >> > Above is simply a wrong statement.
> > > >>
> > > >> The real problem, perhaps, is that there seems to be little point in
> > > >> adding kerneldoc comments for anonymous enums; where are you going to
> > > >> use that documentation?
> > > >
> > > > I had been explicitly told during review (IIRC by maintainers) to make
> > > > it such, while the initial version was exactly like you are thinking
> > > > of. So, I'm not the right person to be asked :-)
> >
> > Just for a reference [1].
> >
> > > >>  The error message could perhaps be changed to
> > > >> say that; meanwhile, perhaps this one could be fixed with an action like
> > > >> s%/**%/*% ?
> > > >
> > > > See above. I think regression comes from the kernel doc script,
> > > > earlier it was okay. That said, the author of kernel doc changes has
> > > > to submit a patch to amend the driver and maintainers will review it.
> > >
> > > kerneldoc now warns about various incorrect things that it used to just
> > > silently pass over.  There is no regression here, just a new diagnostic
> > > to point out something that was never going to work right.  Unless you
> > > have a good idea for what kerneldoc should do with a block like that?
> >
> > As it does, put description of individual fields and prepend it with a
> > common part.
> >
> > So,
> >
> > enum - Bla bla bla
> >  @FOO: ABC
> >  @BAR: DEF
> > Description
> >
> > Should go in the doc for the corresponding file like (as an example)
> >
> > Anonymous enumeration Bla bla bla
> > Description
> >
> > FOO ABC
> > BAR DEF
> >
> > (not sure about indentation, emphasizing and separators, but I think
> > you got the idea).
> >
> > > (An alternative fix, of course, would be to give the enum a name so it
> > > can actually be used for type checking.)
> >
> > That enum is not used as an enum, it provides the logically unified constants.
>
> What's the problem of giving it a name?

Because why should I do this? It's a perfect C language which has no
issues so far.

> You could call it as "intel_pinctrl_pad" or something similar.
>
> > Personally I don't see why the kernel doc can't digest this.
>
> It is not hard to add support for this special case. Just sent a
> patch.

Thanks, I will test it!

> Yet, this adds additional complexity on an script that it is
> already complex enough.

> > [1]: https://patchwork.ozlabs.org/project/linux-gpio/patch/20190808132128.13359-1-andriy.shevchenko@linux.intel.com/

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] scripts/kernel-doc: ignore identifier on anonymous enums

[Jonathan Corbet]

Mauro Carvalho Chehab <mchehab+huawei@kernel.org> writes:

> When anonymous enums are used, the identifier is empty.
>
> While, IMO, it should be avoided the usage of such enums,
> adding support for it is not hard.
>
> So, postpone the check for empty identifiers to happen
> only at the dump phase.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  scripts/kernel-doc | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)

Applied, thanks.

jon