[PATCH 05/13] efi_loader: Move some memory-function comments to header

[Simon Glass]

Hi,

On Tue, 26 Nov 2024 at 16:32, Tom Rini <trini at konsulko.com> wrote:
>
> On Tue, Nov 26, 2024 at 11:50:40PM +0100, Heinrich Schuchardt wrote:
> > On 11/26/24 23:33, Tom Rini wrote:
> > > On Tue, Nov 26, 2024 at 01:04:25PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Tue, 26 Nov 2024 at 08:55, Tom Rini <trini at konsulko.com> wrote:
> > > > >
> > > > > On Tue, Nov 26, 2024 at 08:37:33AM -0700, Simon Glass wrote:
> > > > > > Hi Heinrich,
> > > > > >
> > > > > > On Tue, 26 Nov 2024 at 02:35, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > > > > >
> > > > > > > On 25.11.24 21:44, Simon Glass wrote:
> > > > > > > > Exported functions should be documented in the header file, not the
> > > > > > > > implementation. We tend to make such updates on a piecemeal basis to
> > > > > > > > avoid a 'flag day'. Move some comments related to memory allocation to
> > > > > > > > follow the convention.
> > > > > > > >
> > > > > > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > > > >
> > > > > > > Please, have a look at this line in doc/
> > > > > > >
> > > > > > > doc/api/efi.rst:78:
> > > > > > > .. kernel-doc:: lib/efi_loader/efi_memory.c
> > > > > >
> > > > > > Hmm, we should not add C files as then we end up with all sorts of
> > > > > > internal functions, like checksum(). The help is a bit of a mess on
> > > > > > that page IMO and it could use an index at the top or side.
> > > > >
> > > > > Why? Looking at the linux kernel (where we borrow this framework from
> > > > > and lots of developers expect to follow that style) it's extremely
> > > > > common to kernel-doc C files. I don't see why documenting internal
> > > > > functions (which should be clear as being internal) is a problem.
> > > >
> > > > It depends what you mean by docs. U-Boot puts the documentation for
> > > > exported functions in header files.
> > >
> > > I'm sorry, that's a circular definition. And no, there is not a hard and
> > > fast rule that we only put kernel-doc style comments in header files
> > > (which are consumed by other files and tooling to generate
> > > documentation).
> > >
> > > > Putting docs in C files clutters the docs with internal
> > > > implementations, when most people just want to see the API. The way
> > > > U-Boot does things, it is easy to see the internal (implementation)
> > > > docs by looking at the C code and the external functions by looking at
> > > > the header files.
> > >
> > > Or it makes things harder to understand because the human readable
> > > documentation for a given function is separate from the file the
> > > implements the function. This is possibly why in the linux kernel, where
> > > we borrow "kernel-doc" from, documentation is frequently in the C file
> > > which is then included in Documentation files (the type you objected to
> > > in the start of this patch) which include additional human-readable
> > > explanation of the intention and usage of the API in question.
> > >
> >
> > For static functions we will continue to have function documentation in
> > the C files and not in the headers. I personally never looked up the API
> > documentation in the HTML docs but always in the source.

Same here.

> >
> > I am not opposed against moving the EFI function documentation into the
> > header files. But we should update doc/develop/efi.rst accordingly in
> > the same patch series. This is missing here.

OK

>
> My point is that the linux kernel is about 60% C files and 40% header
> files being kernel-doc'd in to documentation files and so "U-Boot
> doesn't put documentation in to C files" seems like the wrong approach.

U-Boot tries to document functions in header files. At the risk of
making a blanket statement with thousands of counter-examples, that is
not so much the case in Linux.

To a large extent, people developing features in U-Boot rely on the
header files to see what functions they can call. With the
documentation there, they often don't need to look at the source. I
certainly don't, unless the header-file comment is inadequate.

Regards,
SImon