| Message ID | 20170331071632.6209-1-johannes@sipsolutions.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
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 --
> 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
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):
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
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
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
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):