On Mon, 10 Feb 2025 at 10:56, Philippe Mathieu-Daudé <phi...@linaro.org> wrote: > > Hi Peter, > > On 6/2/25 16:12, Peter Maydell wrote: > > -int load_fit(const struct fit_loader *ldr, const char *filename, void > > *opaque); > > +/** > > + * load_fit: load a FIT format image > > + * @ldr: structure defining board specific properties and hooks > > + * @filename: image to load > > + * @pfdt: pointer to update with address of FDT blob > > It is not clear this field is optional or mandatory. > > Looking at other docstrings, optional is not always precised, > and code often consider optional if not precised. Mandatory is > mentioned explicitly.
I did go back and forth while I was writing this on whether to make it optional or not (and the versions where it is optional, I had a note in the documentation about that). But there is exactly one caller of this function, and that callsite wants to pass a non-NULL pointer, so I ended up deciding that it was pointless to make the argument optional and include the code to handle "pfdt is NULL". If you get it wrong you'll find out very quickly because your code will segfault :-) Generally we (should) say arguments are optional when they're optional; in those cases we also should document what the behaviour when they're omitted is. So I think "mandatory" is the default. In this function, ldr and filename also are mandatory. If we mark every mandatory argument as "mandatory" then we will end up with a lot of boilerplate markup for most arguments, I think. > Could you update the documentation? Otherwise consider adding > a non-NULL check. > > Either ways: > Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org> thanks -- PMM
