Re: [PATCH bpf-next] libbpf: Add sphinx code documentation comments

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Thu, Sep 9, 2021 at 1:43 PM grantseltzer <grantseltzer@gmail.com> wrote:
>
> From: Grant Seltzer <grantseltzer@gmail.com>
>
> This adds comments above five functions in btf.h which document
> their uses. These comments are of a format that doxygen and sphinx
> can pick up and render. These are rendered by libbpf.readthedocs.org
>
> Signed-off-by: Grant Seltzer <grantseltzer@gmail.com>
> ---
>  tools/lib/bpf/btf.h | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>

It's nice that you provided a test instance of readthedocs.io site, it
made it much easier to look at how all this is rendered. Thanks!

I'm no technical writer, but left some thoughts below. It would be
great to get more help documenting all the APIs and important libbpf
objects from people that are good at succinctly explaining concepts.
This is a great start!

> diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
> index 4a711f990904..f928e57c238c 100644
> --- a/tools/lib/bpf/btf.h
> +++ b/tools/lib/bpf/btf.h
> @@ -30,11 +30,47 @@ enum btf_endianness {
>         BTF_BIG_ENDIAN = 1,
>  };
>
> +/**
> + * @brief **btf__free** frees all data of the BTF representation

I'd like to propose that we add () for all functions, I've been
roughly following this convention throughout commit messages and
comments in the code, I think it's makes it a bit clearer that we are
talking about functions

> + * @param btf

seems like the format is "<arg_name> <description of the argument>" so
in this case it should be something like

@param btf BTF object to free

> + * @return void

do we need @return if the function is returning void? What if we just
omit @return for such cases?

> + */
>  LIBBPF_API void btf__free(struct btf *btf);
>
> +/**
> + * @brief **btf__new** creates a representation of a BTF section
> + * (struct btf) from the raw bytes of that section

is there some way to cross reference to other types/functions? E.g.,
how do we make `struct btf` a link to its description?

> + * @param data raw bytes
> + * @param size length of raw bytes
> + * @return struct btf*


@return new instance of BTF object which has to be eventually freed
with **btf__free()**?

> + */
>  LIBBPF_API struct btf *btf__new(const void *data, __u32 size);
> +
> +/**
> + * @brief **btf__new_split** creates a representation of a BTF section

"representation of a BTF section" seems a bit mouthful. And it's not a
representation, it's a BTF object that allows to perform a lot of
stuff with BTF type information. So maybe let's describe it as

**btf__new_split()** create a new instance of BTF object from provided
raw data bytes. It takes another BTF instance, **base_btf**, which
serves as a base BTF, which is extended by types in a newly created
BTF instance.

> + * (struct btf) from a combination of raw bytes and a btf struct
> + * where the btf struct provides a basic set of types and strings,
> + * while the raw data adds its own new types and strings
> + * @param data raw bytes
> + * @param size length of raw bytes
> + * @param base_btf the base btf representation

same here, "representation" sounds kind of weird and wrong here

> + * @return struct btf*
> + */
>  LIBBPF_API struct btf *btf__new_split(const void *data, __u32 size, struct btf *base_btf);
> +
> +/**
> + * @brief **btf__new_empty** creates an unpopulated representation of
> + * a BTF section
> + * @return struct btf*
> + */
>  LIBBPF_API struct btf *btf__new_empty(void);
> +
> +/**
> + * @brief **btf__new_empty_split** creates an unpopulated
> + * representation of a BTF section except with a base BTF
> + * ontop of which split BTF should be based

typo: on top

> + * @return struct btf*q
> + */
>  LIBBPF_API struct btf *btf__new_empty_split(struct btf *base_btf);
>
>  LIBBPF_API struct btf *btf__parse(const char *path, struct btf_ext **btf_ext);
> --
> 2.31.1
>