Hi Sughosh,
On Wed, 24 Jul 2024 at 00:03, Sughosh Ganu <sughosh.g...@linaro.org> wrote:
>
> Add a couple of helper functions to detect an empty and full alist.
>
> Signed-off-by: Sughosh Ganu <sughosh.g...@linaro.org>
> ---
> Changes since rfc: None
>
> include/alist.h | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
I had to hunt around to see why these are needed. It's fine to add new
functions to the API, but in this case I want to make a few points.
>
> diff --git a/include/alist.h b/include/alist.h
> index 6cc3161dcd..06ae137102 100644
> --- a/include/alist.h
> +++ b/include/alist.h
> @@ -82,6 +82,28 @@ static inline bool alist_err(struct alist *lst)
> return lst->flags & ALISTF_FAIL;
> }
>
> +/**
> + * alist_full() - Check if the alist is full
> + *
> + * @lst: List to check
> + * Return: true if full, false otherwise
> + */
> +static inline bool alist_full(struct alist *lst)
> +{
> + return lst->count == lst->alloc;
> +}
In general I see you manually modifying the members of the alist,
rather than using the API to add a new item. I think it is better to
use the API.
struct lmb_region rgn;
rgn.base = ...
rgn.size = ...
rgn.flags = ...
if (!alist_add(&lmb.used, rgn. struct lmb_region))
return -ENOMEM;
or you could make a function to add a new region to a list, with base,
size & flags as args.
> +
> +/**
> + * alist_empty() - Check if the alist is empty
> + *
> + * @lst: List to check
> + * Return: true if empty, false otherwise
> + */
> +static inline bool alist_empty(struct alist *lst)
> +{
> + return !lst->count && lst->alloc;
> +}
I would argue that this is a surprising definition of 'empty'. Why the
second term? It seems to be because you want to know if it is safe to
set values in item[0]. But see above for how to use the API.
> +
> /**
> * alist_get_ptr() - Get the value of a pointer
> *
> --
> 2.34.1
>
Regards,
Simon
