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