Re: [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash()

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mar 23, 2023, 19:46 UTC
Message-ID: <d1fb77a0-9ed8-4f3d-5bad-bc443b5522d2@dunelm.org.uk>
In-Reply-To: <20230323162235.995574-7-oswald.buddenhagen@gmx.de>

Hi Oswald

On 23/03/2023 16:22, Oswald Buddenhagen wrote:

> The operation doesn't change the number of elements in the array, so we do
> not need to allocate the result piecewise.

I think the reasoning behind this patch is sound.

Show 38 quoted lines

> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>   sequencer.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index f8a7f4e721..fb224445fa 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -6225,7 +6225,7 @@ static int skip_fixupish(const char *subject, const char **p) {
>   int todo_list_rearrange_squash(struct todo_list *todo_list)
>   {
>   	struct hashmap subject2item;
> -	int rearranged = 0, *next, *tail, i, nr = 0, alloc = 0;
> +	int rearranged = 0, *next, *tail, i, nr = 0;
>   	char **subjects;
>   	struct commit_todo_item commit_todo;
>   	struct todo_item *items = NULL;
> @@ -6334,6 +6334,8 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>   	}
>   
>   	if (rearranged) {
> +		items = ALLOC_ARRAY(items, todo_list->nr);
> +
>   		for (i = 0; i < todo_list->nr; i++) {
>   			enum todo_command command = todo_list->items[i].command;
>   			int cur = i;
> @@ -6346,16 +6348,15 @@ int todo_list_rearrange_squash(struct todo_list *todo_list)
>   				continue;
>   
>   			while (cur >= 0) {
> -				ALLOC_GROW(items, nr + 1, alloc);
>   				items[nr++] = todo_list->items[cur];
>   				cur = next[cur];
>   			}
>   		}
>   
> +		assert(nr == todo_list->nr);

If this assert fails we may have already had some out of bounds memory accesses.

> +		todo_list->alloc = nr;
>   		FREE_AND_NULL(todo_list->items);

I think it would be cleaner to keep the original ordering and free the old list before assigning todo_list->alloc

>   		todo_list->items = items;
> -		todo_list->nr = nr;
> -		todo_list->alloc = alloc;
>   	}

Best Wishes

Phillip

>   	free(next);