Re: [PATCH] utils/fifo8: change fatal errors from abort() to assert()

[Claudio Fontana <cfontana@suse.de>]

On 1/14/21 10:58 AM, Mark Cave-Ayland wrote:
> On 14/01/2021 09:07, Claudio Fontana wrote:
> 
>> On 1/14/21 9:33 AM, Mark Cave-Ayland wrote:
>>> Developer errors are better represented with assert() rather than abort().
>>
>> ... "also, make the tests more strict"
>>
>> I'd add this since the checks have been changed sometimes in the patch to be more strict.
>>
>> Reviewed-by: Claudio Fontana <cfontana@suse.de>
> 
> Oh, that was not intentional on my part - I was aiming to keep the same logic but 
> effectively invert the logic to keep the assert() happy. What did I miss?

Did I misunderstand? Comments below:

> 
> 
> ATB,
> 
> Mark.
> 
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> This was suggested by Peter during a discussion on IRC yesterday.
>>>
>>> ---
>>>   util/fifo8.c | 16 ++++------------
>>>   1 file changed, 4 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/util/fifo8.c b/util/fifo8.c
>>> index a5dd789ce5..d4d1c135e0 100644
>>> --- a/util/fifo8.c
>>> +++ b/util/fifo8.c
>>> @@ -31,9 +31,7 @@ void fifo8_destroy(Fifo8 *fifo)
>>>   
>>>   void fifo8_push(Fifo8 *fifo, uint8_t data)
>>>   {
>>> -    if (fifo->num == fifo->capacity) {
>>> -        abort();
>>> -    }
>>> +    assert(fifo->num < fifo->capacity);

This changes the check effectively, the same logic would be in my view:

assert(fifo->num != fifo->capacity);

But I think your change actually makes sense.

>>>       fifo->data[(fifo->head + fifo->num) % fifo->capacity] = data;
>>>       fifo->num++;
>>>   }
>>> @@ -42,9 +40,7 @@ void fifo8_push_all(Fifo8 *fifo, const uint8_t *data, uint32_t num)
>>>   {
>>>       uint32_t start, avail;
>>>   
>>> -    if (fifo->num + num > fifo->capacity) {
>>> -        abort();
>>> -    }
>>> +    assert(fifo->num + num <= fifo->capacity);
>>>   
>>>       start = (fifo->head + fifo->num) % fifo->capacity;
>>>   
>>> @@ -63,9 +59,7 @@ uint8_t fifo8_pop(Fifo8 *fifo)
>>>   {
>>>       uint8_t ret;
>>>   
>>> -    if (fifo->num == 0) {
>>> -        abort();
>>> -    }
>>> +    assert(fifo->num > 0);


applying the exact same logic would be:

assert(fifo->num != 0);

but again, I think that the actual change is more expressive, and most likely is correct, just more strict.


>>>       ret = fifo->data[fifo->head++];
>>>       fifo->head %= fifo->capacity;
>>>       fifo->num--;
>>> @@ -76,9 +70,7 @@ const uint8_t *fifo8_pop_buf(Fifo8 *fifo, uint32_t max, uint32_t *num)
>>>   {
>>>       uint8_t *ret;
>>>   
>>> -    if (max == 0 || max > fifo->num) {
>>> -        abort();
>>> -    }
>>> +    assert(max > 0 && max <= fifo->num);
>>>       *num = MIN(fifo->capacity - fifo->head, max);
>>>       ret = &fifo->data[fifo->head];
>>>       fifo->head += *num;
>>>
>>
>>
> 

Ciao,

Claudio