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

From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Mar 23, 2023, 22:13 UTC
Message-ID: <ZBzPIPQ+GlnPo7Mj@ugly>
In-Reply-To: <d1fb77a0-9ed8-4f3d-5bad-bc443b5522d2@dunelm.org.uk>

On Thu, Mar 23, 2023 at 07:46:28PM +0000, Phillip Wood wrote:

Show 5 quoted lines

>> +		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.

Show 6 quoted lines

>> +		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.

Show 5 quoted lines

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