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