|
| From: | Mark Cave-Ayland |
| Subject: | Re: [PATCH] utils/fifo8: change fatal errors from abort() to assert() |
| Date: | Thu, 21 Jan 2021 09:50:04 +0000 |
| User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 |
On 14/01/2021 10:15, Claudio Fontana wrote:
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.
Got it - the difference between using a range check instead of an inequality check :)
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.
Agreed. In theory both forms should be the same since these elements are integers, but I do also prefer being explicit about it being a numeric range.
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;
I'll submit a v2 shortly adding your R-B. ATB, Mark.
| [Prev in Thread] | Current Thread | [Next in Thread] |