[PATCH 1/2] Documentation/sphinx: kerneldoc: add "unused-functions"

[PATCH 1/2] Documentation/sphinx: kerneldoc: add "unused-functions"

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

When adding functions one by one into documentation, in order to
order/group things properly, it's easy to miss things. Allow use
of the kernel-doc directive with "unused-functions" like this

.. kernel-doc:: <filename>
   :unused-functions:

to output anything previously unused from that file. This allows
grouping things but still making sure that the documentation has
all the functions.

Internally this works by collecting (per-file) those functions
(and enums, structs, doc sections...) that are explicitly used,
and invoking the kernel-doc script with "-nofunction" later.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 Documentation/sphinx/kerneldoc.py | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
index d15e07f36881..79fc1491348a 100644
--- a/Documentation/sphinx/kerneldoc.py
+++ b/Documentation/sphinx/kerneldoc.py
@@ -41,6 +41,9 @@ from sphinx.ext.autodoc import AutodocReporter
 
 __version__  = '1.0'
 
+# per-file list
+_used_fns = {}
+
 class KernelDocDirective(Directive):
     """Extract kernel-doc comments from the specified file"""
     required_argument = 1
@@ -50,6 +53,7 @@ class KernelDocDirective(Directive):
         'functions': directives.unchanged_required,
         'export': directives.unchanged,
         'internal': directives.unchanged,
+        'unused-functions': directives.unchanged,
     }
     has_content = False
 
@@ -60,6 +64,10 @@ class KernelDocDirective(Directive):
         filename = env.config.kerneldoc_srctree + '/' + self.arguments[0]
         export_file_patterns = []
 
+        if not filename in _used_fns:
+            _used_fns[filename] = []
+        _used_fns_this_file = _used_fns[filename]
+
         # Tell sphinx of the dependency
         env.note_dependency(os.path.abspath(filename))
 
@@ -73,10 +81,16 @@ class KernelDocDirective(Directive):
             cmd += ['-internal']
             export_file_patterns = str(self.options.get('internal')).split()
         elif 'doc' in self.options:
-            cmd += ['-function', str(self.options.get('doc'))]
+            f = str(self.options.get('doc'))
+            cmd += ['-function', f]
+            _used_fns_this_file.append(f)
+        elif 'unused-functions' in self.options:
+            for f in _used_fns_this_file:
+                cmd += ['-nofunction', f]
         elif 'functions' in self.options:
             for f in str(self.options.get('functions')).split():
                 cmd += ['-function', f]
+                _used_fns_this_file.append(f)
 
         for pattern in export_file_patterns:
             for f in glob.glob(env.config.kerneldoc_srctree + '/' + pattern):
-- 
2.11.0

[PATCH 2/2] cfg80211: add remaining functions/etc. to documentation

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

A lot (likely most!) enums, structs and functions aren't explicitly
listed in the documentation. For now, just add everything to the
documentation in a new section so at least the references to those
functions can be resolved.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 Documentation/driver-api/80211/cfg80211.rst | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/driver-api/80211/cfg80211.rst b/Documentation/driver-api/80211/cfg80211.rst
index 8ffac57e1f5b..52caec2c9ff9 100644
--- a/Documentation/driver-api/80211/cfg80211.rst
+++ b/Documentation/driver-api/80211/cfg80211.rst
@@ -355,3 +355,8 @@ Test mode
 
 .. kernel-doc:: include/net/cfg80211.h
    :functions: cfg80211_testmode_event
+
+Remaining functions
+===================
+.. kernel-doc:: include/net/cfg80211.h
+   :unused-functions:
-- 
2.11.0

Re: [PATCH 1/2] Documentation/sphinx: kerneldoc: add "unused-functions"

[Markus Heiser]

Am 31.03.2017 um 09:16 schrieb Johannes Berg <johannes@sipsolutions.net>:

> From: Johannes Berg <johannes.berg@intel.com>
> 
> When adding functions one by one into documentation, in order to
> order/group things properly, it's easy to miss things. Allow use
> of the kernel-doc directive with "unused-functions" like this
> 
> .. kernel-doc:: <filename>
>  :unused-functions:
> 
> to output anything previously unused from that file. This allows
> grouping things but still making sure that the documentation has
> all the functions.

Do we really need such generic stuff? ... IMO explicit is better than
implicit. Why not getting an error when a function, which is referred
from a reST-document disappears in the source? Those errors help
to maintain the consistency of documentation with source-code.

In the past (DocBook) we had such generic stuff and IMO it was not
helpful to serve consistency. Take a look at the old DocBook stuff,
most of it is outdated and some object in the source-code, which are
referred in the past from DocBooks, are no longer existing ... but no
errors when compiling the DocBooks, because these are all generic
includes.

I know, there are also use-cases where generic is very helpful (e.g.
create a complete API description from the header file, with just
one line in reST). And I know, that we have already generic e.g. the
"export" option of the kernel-doc directive.

I'am not totally against generic, but I think every decision in
this direction should be well considered.

These are only my 2cent.

-- Markus --

Re: [PATCH 1/2] Documentation/sphinx: kerneldoc: add "unused-functions"

[Johannes Berg]

> Do we really need such generic stuff? ... IMO explicit is better than
> implicit. Why not getting an error when a function, which is referred
> from a reST-document disappears in the source? Those errors help
> to maintain the consistency of documentation with source-code.

That's a totally different problem.

> I know, there are also use-cases where generic is very helpful (e.g.
> create a complete API description from the header file, with just
> one line in reST). And I know, that we have already generic e.g. the
> "export" option of the kernel-doc directive.

Exactly. But now you can either

 * use "export" or "internal" to get *everything*
 * list every single function, and get no warning when there's a
   function you didn't list

This serves to help get a mixture of the two, to be able to group
things but also document everything that got missed as a fall-back.

johannes

Re: [PATCH 1/2] Documentation/sphinx: kerneldoc: add "unused-functions"

[Jani Nikula]

On Fri, 31 Mar 2017, Johannes Berg <johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> When adding functions one by one into documentation, in order to
> order/group things properly, it's easy to miss things. Allow use
> of the kernel-doc directive with "unused-functions" like this
>
> .. kernel-doc:: <filename>
>    :unused-functions:

I'm sure the parameter name could be improved to capture what you mean
better; alas I don't have a suggestion.

>
> to output anything previously unused from that file. This allows
> grouping things but still making sure that the documentation has
> all the functions.
>
> Internally this works by collecting (per-file) those functions
> (and enums, structs, doc sections...) that are explicitly used,
> and invoking the kernel-doc script with "-nofunction" later.

A quick thought that I don't have the time to check now, but should be
checked before merging: Is the order of directive extension execution
deterministic if the Sphinx run is parallelized (sphinx-build -j)? Is it
deterministic within an rst file? Surely it's not deterministic when
called from several rst files? The latter is, perhaps, acceptable, but
the former not.

BR,
Jani.


>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  Documentation/sphinx/kerneldoc.py | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
> index d15e07f36881..79fc1491348a 100644
> --- a/Documentation/sphinx/kerneldoc.py
> +++ b/Documentation/sphinx/kerneldoc.py
> @@ -41,6 +41,9 @@ from sphinx.ext.autodoc import AutodocReporter
>  
>  __version__  = '1.0'
>  
> +# per-file list
> +_used_fns = {}
> +
>  class KernelDocDirective(Directive):
>      """Extract kernel-doc comments from the specified file"""
>      required_argument = 1
> @@ -50,6 +53,7 @@ class KernelDocDirective(Directive):
>          'functions': directives.unchanged_required,
>          'export': directives.unchanged,
>          'internal': directives.unchanged,
> +        'unused-functions': directives.unchanged,
>      }
>      has_content = False
>  
> @@ -60,6 +64,10 @@ class KernelDocDirective(Directive):
>          filename = env.config.kerneldoc_srctree + '/' + self.arguments[0]
>          export_file_patterns = []
>  
> +        if not filename in _used_fns:
> +            _used_fns[filename] = []
> +        _used_fns_this_file = _used_fns[filename]
> +
>          # Tell sphinx of the dependency
>          env.note_dependency(os.path.abspath(filename))
>  
> @@ -73,10 +81,16 @@ class KernelDocDirective(Directive):
>              cmd += ['-internal']
>              export_file_patterns = str(self.options.get('internal')).split()
>          elif 'doc' in self.options:
> -            cmd += ['-function', str(self.options.get('doc'))]
> +            f = str(self.options.get('doc'))
> +            cmd += ['-function', f]
> +            _used_fns_this_file.append(f)
> +        elif 'unused-functions' in self.options:
> +            for f in _used_fns_this_file:
> +                cmd += ['-nofunction', f]
>          elif 'functions' in self.options:
>              for f in str(self.options.get('functions')).split():
>                  cmd += ['-function', f]
> +                _used_fns_this_file.append(f)
>  
>          for pattern in export_file_patterns:
>              for f in glob.glob(env.config.kerneldoc_srctree + '/' + pattern):

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/2] Documentation/sphinx: kerneldoc: add "unused-functions"

[Johannes Berg]

On Fri, 2017-03-31 at 15:54 +0300, Jani Nikula wrote:
> 
> I'm sure the parameter name could be improved to capture what you
> mean better; alas I don't have a suggestion.

Yes, that's a fair point - perhaps "functions-not-linked" or something
like that.

> > Internally this works by collecting (per-file) those functions
> > (and enums, structs, doc sections...) that are explicitly used,
> > and invoking the kernel-doc script with "-nofunction" later.
> 
> A quick thought that I don't have the time to check now, but should
> be checked before merging: Is the order of directive extension
> execution deterministic if the Sphinx run is parallelized (sphinx-
> build -j)? Is it deterministic within an rst file? Surely it's not
> deterministic when called from several rst files? The latter is,
> perhaps, acceptable, but the former not.

Interesting, TBH I never even considered this. How would I even run it
that way? Presumably "make htmldocs" doesn't do this?

Sphinx documentation (http://www.sphinx-doc.org/en/stable/extdev/) says
this:

    The setup() function can return a dictionary. This is treated by
    Sphinx as metadata of the extension. Metadata keys currently
    recognized are:
    [...]
    'parallel_read_safe': a boolean that specifies if parallel reading
    of source files can be used when the extension is loaded. It
    defaults to False, i.e. you have to explicitly specify your
    extension to be parallel-read-safe after checking that it is.

    We do set this right now, so I guess it'd only be guaranteed to work
    right within a single rst file, and then I should perhaps consider not
    making this state global but somehow linking it to the rst file being
    processed?

    johannes

Re: [PATCH 1/2] Documentation/sphinx: kerneldoc: add "unused-functions"

[Jani Nikula]

On Mon, 03 Apr 2017, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2017-03-31 at 15:54 +0300, Jani Nikula wrote:
>> 
>> I'm sure the parameter name could be improved to capture what you
>> mean better; alas I don't have a suggestion.
>
> Yes, that's a fair point - perhaps "functions-not-linked" or something
> like that.
>
>> > Internally this works by collecting (per-file) those functions
>> > (and enums, structs, doc sections...) that are explicitly used,
>> > and invoking the kernel-doc script with "-nofunction" later.
>> 
>> A quick thought that I don't have the time to check now, but should
>> be checked before merging: Is the order of directive extension
>> execution deterministic if the Sphinx run is parallelized (sphinx-
>> build -j)? Is it deterministic within an rst file? Surely it's not
>> deterministic when called from several rst files? The latter is,
>> perhaps, acceptable, but the former not.
>
> Interesting, TBH I never even considered this. How would I even run it
> that way? Presumably "make htmldocs" doesn't do this?

Try 'make SPHINXOPTS=-j8 htmldocs'.

>
> Sphinx documentation (http://www.sphinx-doc.org/en/stable/extdev/) says
> this:
>
>     The setup() function can return a dictionary. This is treated by
>     Sphinx as metadata of the extension. Metadata keys currently
>     recognized are:
>     [...]
>     'parallel_read_safe': a boolean that specifies if parallel reading
>     of source files can be used when the extension is loaded. It
>     defaults to False, i.e. you have to explicitly specify your
>     extension to be parallel-read-safe after checking that it is.
>
>     We do set this right now, so I guess it'd only be guaranteed to work
>     right within a single rst file, and then I should perhaps consider not
>     making this state global but somehow linking it to the rst file being
>     processed?

Perhaps, but does that defeat the purpose then?

BR,
Jani.

>
>     johannes
> --
> To unsubscribe from this list: send the line "unsubscribe linux-doc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/2] Documentation/sphinx: kerneldoc: add "unused-functions"

[Johannes Berg]

On Tue, 2017-04-04 at 10:26 +0300, Jani Nikula wrote:
> 
> > Interesting, TBH I never even considered this. How would I even run
> > it that way? Presumably "make htmldocs" doesn't do this?
> 
> Try 'make SPHINXOPTS=-j8 htmldocs'.

Yep, makes sense.

> > Sphinx documentation (http://www.sphinx-doc.org/en/stable/extdev/) says
> > this:
> > 
> >     The setup() function can return a dictionary. This is treated by
> >     Sphinx as metadata of the extension. Metadata keys currently
> >     recognized are:
> >     [...]
> >     'parallel_read_safe': a boolean that specifies if parallel reading
> >     of source files can be used when the extension is loaded. It
> >     defaults to False, i.e. you have to explicitly specify your
> >     extension to be parallel-read-safe after checking that it is.
> > 
> >     We do set this right now, so I guess it'd only be guaranteed to work
> >     right within a single rst file, and then I should perhaps consider not
> >     making this state global but somehow linking it to the rst file being
> >     processed?
> 
> Perhaps, but does that defeat the purpose then?

Yeah, it kinda does. For my original use case in cfg80211 we only have
a single file, but even in mac80211 we already use more than one.

Not sure what to do then - I guess we just can't do that, unless we
prevent using this with parallelization, which seems awkward.

johannes