From: Phillip Wood Date: Thu, 23 Mar 2023 19:46:28 GMT Subject: Re: [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash() Message-ID: 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. > Signed-off-by: Oswald Buddenhagen > --- > 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);