| Message ID | 20230323162235.995574-7-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sequencer refactoring | expand |
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 <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);
On Thu, Mar 23, 2023 at 07:46:28PM +0000, Phillip Wood wrote: >> + assert(nr == todo_list->nr); > >If this assert fails we may have already had some out of bounds memory >accesses. > the loop could have run short, too. but anyway, this isn't a runtime check, it's an assertion of a loop invariant. >> + 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 > my reasoning is that it's closer to the assert which also refers to it, and it really makes sense to have _that_ first. also, the value is more likely to be still in a register at that point. >> todo_list->items = items; >> - todo_list->nr = nr; >> - todo_list->alloc = alloc; >> } >
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); + todo_list->alloc = nr; FREE_AND_NULL(todo_list->items); todo_list->items = items; - todo_list->nr = nr; - todo_list->alloc = alloc; } free(next);
The operation doesn't change the number of elements in the array, so we do not need to allocate the result piecewise. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- sequencer.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)