From: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Date: Thu, 23 Mar 2023 22:13:52 GMT
Subject: Re: [PATCH 6/8] sequencer: simplify allocation of result array in todo_list_rearrange_squash()
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:
>> +		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;
>>   	}
>