Re: [PATCH] kernel-doc: better handle '::' sequences

From: Jani Nikula
Date: Tue Mar 30 2021 - 07:37:02 EST


On Mon, 29 Mar 2021, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> On Mon, Mar 29, 2021 at 09:33:30PM +0300, Jani Nikula wrote:
>> On Mon, 29 Mar 2021, Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>> > So here's my "modest proposal":
>> >
>> > - Similar to our ".. kernel-doc::" invocation in .rst files, handle
>> > ".. rustdoc::" (insert weeks of hacking here)
>> > - Now add ".. rst-doc::" which parses .c files like [1] kernel-doc
>> > does, but interprets a different style of comment and actually does
>> > most of the repetitive boring bits for you.
>>
>> As a hobby, I've written a Sphinx extension to use Clang to parse the
>> code and extract pure reStructuredText documentation comments with
>> minimal conversions [1]. No additional syntax. Just use reStructuredText
>> for everything instead of inventing your own.
>>
>> I'm not proposing to use that in kernel, at all. It was more like a
>> diversion from the kernel documentation.
>
> Actually, that looks like my proposal, except that it uses the same /**
> as kernel-doc, so you can't tell whether a comment is intended to be
> interpreted by kernel-doc or hawkmoth.
>
> https://github.com/jnikula/hawkmoth/blob/master/test/example-70-function.c
>
> If the introduction were "/*rST" instead of "/**", would we have
> consensus? It gives us a path to let people intermix kernel-doc and
> hawkmoth comments in the same file, which would be amazing.

If you want to allow two syntaxes for documentation comments (current
kernel-doc and pure reStructuredText with just the comment markers and
indentation removed) I think the natural first step would be to modify
kernel-doc the perl script to support that. It would probably even be
trivial.

Hawkmoth uses Clang for parsing, with none of the kernel specific stuff
that kernel-doc has, such as EXPORT_SYMBOL(). It makes sense for a pet
project with a clean break. I don't know if anyone has the bandwidth or
desire to re-implement the kernel specific stuff on top of Clang. (I
know I don't, I started the project because I wanted that clean break to
begin with!)

The real question is, is it a good idea to support multiple formats at
all? (N.b. I'm not a fan of extending the kernel-doc syntax either.)

BR,
Jani.


>
>> But based on my experience with the old and new kernel documentation
>> systems and the hobby one, the one takeaway is to not create new
>> syntaxes, grammars, parsers, or preprocessors to be maintained by the
>> kernel community. Just don't. Take what's working and supported by other
>> projects, and add the minimal glue using Sphinx extensions to put it
>> together, and no more.
>>
>> Of course, we couldn't ditch kernel-doc the script, but we managed to
>> trim it down quite a bit. OTOH, there have been a number of additions
>> outside of Sphinx in Makefiles and custom tools in various languages
>> that I'm really not happy about. It's all too reminiscient of the old
>> DocBook toolchain, while Sphinx was supposed to be the one tool to tie
>> it all together, partially chosen because of the extension support.
>>
>>
>> BR,
>> Jani.
>>
>>
>> [1] https://github.com/jnikula/hawkmoth
>>
>>
>> >
>> > For example, xa_load:
>> >
>> > /**
>> > * xa_load() - Load an entry from an XArray.
>> > * @xa: XArray.
>> > * @index: index into array.
>> > *
>> > * Context: Any context. Takes and releases the RCU lock.
>> > * Return: The entry at @index in @xa.
>> > */
>> > void *xa_load(struct xarray *xa, unsigned long index)
>> >
>> > //rST
>> > // Load an entry from an XArray.
>> > //
>> > // :Context: Any context. Takes and releases the RCU lock.
>> > // :Return: The entry in `xa` at `index`.
>> > void *xa_load(struct xarray *xa, unsigned long index)
>> >
>> > (more complex example below [2])
>> >
>> > Things I considered:
>> >
>> > - Explicitly document that this is rST markup instead of Markdown or
>> > whatever.
>> > - Don't repeat the name of the function. The tool can figure it out.
>> > - Don't force documenting each parameter. Often they are obvious
>> > and there's really nothing interesting to say about the parameter.
>> > Witness the number of '@foo: The foo' (of type struct foo) that we
>> > have scattered throughout the tree. It's not that the documenter is
>> > lazy, it's that there's genuinely nothing to say here.
>> > - Use `interpreted text` to refer to parameters instead of *emphasis* or
>> > **strong emphasis**. The tool can turn that into whatever markup
>> > is appropriate.
>> > - Use field lists for Context and Return instead of sections. The markup
>> > is simpler to use, and I think the rendered output is better.
>> >
>> > [1] by which i mean "in a completely different way from, but similar in
>> > concept"
>> >
>> > [2] More complex example:
>> >
>> > /**
>> > * xa_store() - Store this entry in the XArray.
>> > * @xa: XArray.
>> > * @index: Index into array.
>> > * @entry: New entry.
>> > * @gfp: Memory allocation flags.
>> > *
>> > * After this function returns, loads from this index will return @entry.
>> > * Storing into an existing multi-index entry updates the entry of every index.
>> > * The marks associated with @index are unaffected unless @entry is %NULL.
>> > *
>> > * Context: Any context. Takes and releases the xa_lock.
>> > * May sleep if the @gfp flags permit.
>> > * Return: The old entry at this index on success, xa_err(-EINVAL) if @entry
>> > * cannot be stored in an XArray, or xa_err(-ENOMEM) if memory allocation
>> > * failed.
>> > */
>> > void *xa_store(struct xarray *xa, unsigned long index, void *entry, gfp_t gfp)
>> >
>> > //rST
>> > // Store an entry in the XArray.
>> > //
>> > // After this function returns, loads from `index` will return `entry`.
>> > // Storing into an existing multi-index entry updates the entry of every index.
>> > // The marks associated with `index` are unaffected unless `entry` is ``NULL``.
>> > //
>> > // :Context: Any context. Takes and releases the xa_lock.
>> > // May sleep if the `gfp` flags permit.
>> > // :Return: The old entry at this index on success, xa_err(-EINVAL) if `entry`
>> > // cannot be stored in an XArray, or xa_err(-ENOMEM) if memory allocation
>> > // failed.
>> > void *xa_store(struct xarray *xa, unsigned long index, void *entry, gfp_t gfp)
>> >
>>
>> --
>> Jani Nikula, Intel Open Source Graphics Center

--
Jani Nikula, Intel Open Source Graphics Center