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

Add a couple of helper functions to detect an empty and full alist.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since rfc: None

 include/alist.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Hi Sughosh,

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

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

Yes, I had seen this usage of the API in another of your patch.
However, my personal opinion of the API's is that alist_expand_by()
gives more clarity on what the function is doing. One gets a feeling
that alist_add() is only adding the given node to the list, whereas it
is doing a lot more under the hood. I feel that using the
alist_expand_by() API makes the code much easier to understand, but
that's my personal opinion.

>
> 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.

I want the initialisation of the list to be kept separate from adding
more 'nodes' to the list. Hence my usage.

-sughosh

>
> > +
> >  /**
> >   * alist_get_ptr() - Get the value of a pointer
> >   *
> > --
> > 2.34.1
> >
>
> Regards,
> Simon

Hi Sughosh,

On Mon, 29 Jul 2024 at 12:05, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> On Fri, 26 Jul 2024 at 05:03, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Wed, 24 Jul 2024 at 00:03, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > Add a couple of helper functions to detect an empty and full alist.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@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;
>
> Yes, I had seen this usage of the API in another of your patch.
> However, my personal opinion of the API's is that alist_expand_by()
> gives more clarity on what the function is doing. One gets a feeling
> that alist_add() is only adding the given node to the list, whereas it
> is doing a lot more under the hood. I feel that using the
> alist_expand_by() API makes the code much easier to understand, but
> that's my personal opinion.

With lmb there is the case of inserting a new element into the array,
something which the API doesn't support at present. Perhaps that would
help? It is a little hard to design the API until there are more users
of it.

alist_add() is really just adding an element (not a node) to the list,
isn't it? Do you mean that it is copying the data into the list?

>
> >
> > 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.
>
> I want the initialisation of the list to be kept separate from adding
> more 'nodes' to the list. Hence my usage.

In that case, please rename the function to something other than 'empty'.

Regards,
Simon