troydhanson / uthash

C macros for hash tables and more
Other
4.18k stars 926 forks source link

LL_DELETE crashes on empty list #196

Closed danadam closed 3 years ago

danadam commented 4 years ago

Macros for append and prepend (obviously), concat, insert inorder, lower bound, count, for each, search scalar, search work well with empty lists (i.e. head == NULL).

Deleting non-existing element from non-empty list works well.

But trying to delete an element from an empty list crashes.

I'm a bit confused if this is intended behavior? On the one hand all those other operations work well with empty lists, on the other hand, DL_DELETE has explicit assert that head must not be NULL.

Quuxplusone commented 4 years ago

Deleting non-existing element from non-empty list works well.

If by that you mean that you're calling DL_DELETE(head, del) where del points to an object that is not actually an element of head... that's out-of-contract and is not expected to work. Just like C++'s std::list<T>::erase(iterator) is only meant to erase an item from the list it's actually in, DL_DELETE and LL_DELETE are only meant to erase an item from the list it's actually in.

Because of this contract, it logically follows that calling DL_DELETE(NULL, del) or LL_DELETE(NULL, del) is always a bug: an empty list contains no elements, so it's never in-contract to delete any element from an empty list.

As for why DL_DELETE2 asserts its contract, but LL_DELETE2 and CDL_DELETE2 do not: The assert was added in that one place in 2017 specifically to fix a user-reported false-positive diagnostic from Clang's static analyzer. See commit 693a09dbd9e22c534f03e8abc6a1bd8cdec12261. I don't know if it's still needed, and I don't know if Clang would have complained equally about LL_DELETE2 and CDL_DELETE2 if the user had tried to use them.

So I'm inclined to say that there's no work to do here, unless you think the existing docs are confusing and want to submit a patch to improve them?