[PATCH 06/40] alist: add a couple of helper functions

[Simon Glass]

Hi Sughosh,

On Wed, 24 Jul 2024 at 00:03, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> Add a couple of helper functions to detect an empty and full alist.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu at 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