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

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

[Jani Nikula]

On Mon, 03 Apr 2017, Johannes Berg  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 majord...@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"

[Jani Nikula]

On Fri, 31 Mar 2017, Johannes Berg  wrote:
> From: Johannes Berg 
>
> 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:: 
>: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 
> ---
>  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"

[Markus Heiser]

Am 31.03.2017 um 09:16 schrieb Johannes Berg :

> From: Johannes Berg 
> 
> 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:: 
>  :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

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

[Johannes Berg]

From: Johannes Berg 

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:: 
   :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 
---
 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