On Fri, 26 Jul 2024 at 05:03, Simon Glass <s...@chromium.org> wrote:
>
> 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;
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
