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

Mon Dec 2 14:35:07 CET 2024

On Sat, Nov 30, 2024 at 01:24:54PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 27 Nov 2024 at 15:11, Tom Rini <trini at konsulko.com> wrote:
> >
> > On Wed, Nov 27, 2024 at 07:56:11AM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Wed, 27 Nov 2024 at 07:15, Tom Rini <trini at konsulko.com> wrote:
> > > >
> > > > On Wed, Nov 27, 2024 at 06:07:58AM -0700, Simon Glass wrote:
> > > > > 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.
> > > >
> > > > I think this is a case of what you do, rather than what everyone else
> > > > does.
> > >
> > > That's not my reading of the source tree.
> >
> > So, if we call this point 1 for a moment.
> >
> > > The file we are talking about has some comments in the header and some
> > > in the source file. When you use an lsp client, it doesn't know what
> > > to do.
> >
> > Sounds like a bad client? Because again, we should be following the
> > kernel here.
> >
> > > > We want to be more like the linux kernel here, rather than less
> > > > like the linux kernel, given the number of kernel people also working
> > > > here. If Heinrich is fine with re-arranging everything, OK. But I think
> > > > documentation with the code makes it less likely to get out of date,
> > > > both in terms of intentional changes and bug fixes.
> > >
> > > I am not suggesting separating documentation from code. I agree that
> > > makes no sense. What I have consistently done (and asked for in
> > > reviews) is:
> > >
> > > - exported functions are documented next to their prototype in the header
> > > - static functions are documented next to their implementation
> >
> > So going back to point 1 now, yes. This is what I'm saying. Your
> > preference, expressed through reviews and telling people to change when
> > they do it other ways, is where the current situation comes from.
> >
> > > This makes it easy to read the header file and see what the module is
> > > doing, without having to dive into the implementation, which could be
> > > 1000 lines or more. It also makes for more useful kerneldoc, IMO,
> > > since we are showing functions which are exported and therefore more
> > > useful for development. Adding the entire C code of U-Boot into
> > > kerneldoc seems like a bad use of space.
> >
> > Um, I don't follow you here. If the code doesn't have kerneldoc
> > comments, it's not included. I just made a silly test with a change to
> > include/clk.h. It's not included in the output. And "the code might be
> > so big it needs refactoring" is not a good excuse.
> 
> Yes, I see that static functions don't appear by default, so adding C
> files doesn't necessarily result in functions appearing in the doc
> output.

Only properly formatted comments are used in documentation, yes. So
static functions could be, if it was important to do so.

> > The kernel is inconsistent here. The first example I went to look for of
> > something that uses both C and header files in a document was
> > Documentation/admin-guide/pstore-blk.rst and both fs/pstore/blk.c and
> > include/linux/pstore_blk.h are kernel-doc'd in and the header describes
> > a public function of the C file while the C file describes other
> > functions too.
> 
> Yes, pstore docs seems like a nice way to do things, for docs.
> 
> For people working on the code base, I strongly believe that exported
> functions and structs are the main way to understand how to use a
> subsystem, so their docs should be in the header file.

Sure. But there is no project-wide rule. I strongly believe that the API
should be documented alongside of the code so that it doesn't drift
apart, among other reasons.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20241202/5dd5eefd/attachment.sig>