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

[Simon Glass]

Hi Sughosh,

On Mon, 29 Jul 2024 at 12:05, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> On Fri, 26 Jul 2024 at 05:03, Simon Glass <sjg at chromium.org> wrote:
> >
> > 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;
>
> 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