Re: [PATCH 12/16] mktree: use iterator struct to add tree entries to index
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jun 12, 2024, 09:40 UTC
- Message-ID
- <ZmltI7HA7O4w2E-6@tanuki>
- In-Reply-To
- <94d6615d634c4f78c88d3e01abbb27f13f85828c.1718130288.git.gitgitgadget@gmail.com>
On Tue, Jun 11, 2024 at 06:24:44PM +0000, Victoria Dye via GitGitGadget wrote:
Show 37 quoted lines
> From: Victoria Dye <vdye@github.com>
>
> Create 'struct tree_entry_iterator' to manage iteration through a 'struct
> tree_entry_array'. Using an iterator allows for conditional iteration; this
> functionality will be necessary in later commits when performing parallel
> iteration through multiple sets of tree entries.
>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> builtin/mktree.c | 40 +++++++++++++++++++++++++++++++++++++---
> 1 file changed, 37 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/mktree.c b/builtin/mktree.c
> index 12f68187221..bee359e9978 100644
> --- a/builtin/mktree.c
> +++ b/builtin/mktree.c
> @@ -137,6 +137,38 @@ static void sort_and_dedup_tree_entry_array(struct tree_entry_array *arr)
> QSORT_S(arr->entries, arr->nr, ent_compare, &ignore_mode);
> }
>
> +struct tree_entry_iterator {
> + struct tree_entry *current;
> +
> + /* private */
> + struct {
> + struct tree_entry_array *arr;
> + size_t idx;
> + } priv;
> +};
> +
> +static void init_tree_entry_iterator(struct tree_entry_iterator *iter,
> + struct tree_entry_array *arr)
> +{
> + iter->priv.arr = arr;
> + iter->priv.idx = 0;
> + iter->current = 0 < arr->nr ? arr->entries[0] : NULL;
> +}Nit: Same comment as before, I think these should rather be named `tree_entry_iterator_init()` and `tree_entry_iterator_advance()`.
Show 13 quoted lines
> +/*
> + * Advance the tree entry iterator to the next entry in the array. If no entries
> + * remain, 'current' is set to NULL. Returns the previous 'current' value of the
> + * iterator.
> + */
> +static struct tree_entry *advance_tree_entry_iterator(struct tree_entry_iterator *iter)
> +{
> + struct tree_entry *prev = iter->current;
> + iter->current = (iter->priv.idx + 1) < iter->priv.arr->nr
> + ? iter->priv.arr->entries[++iter->priv.idx]
> + : NULL;
> + return prev;
> +}I think it's somewhat confusing to have this return a different value than `current`. When I call `next()`, then I expect the iterator to return the next item. And after having called `next()`, I expect that the current value is the one that the previous call to `next()` has returned.
To avoid confusion, I'd propose to get rid of the `current` member altogether. It's not needed as we already save the current index and avoids the confusion.
Patrick