Hi Eric,
> - if (! (bucket < table->bucket_limit))
> + if (! (bucket && bucket < table->bucket_limit))
> abort ();
I would not apply this, because it causes a slowdown at runtime
for no good reason.
I think the paragraph that Paul cited just three hours ago
"Don't make the program ugly to placate `lint'. Please don't insert
any casts to `void'. Zero without a cast is perfectly fine as a null
pointer constant, except when calling a varargs function."
in a certain sense also applies to 'clang'. Don't make the program
ugly or less efficient to placate 'clang', because it's not a tool that
we use daily.
> Clang assumed that the for loop at line 310 is skipped because
> cursor is NULL, which implies bucket is NULL, which implies
> that line 316 bucket->data is a dereference near NULL. But
> this is invalid, because bucket was explicitly initialized
> to table->bucket (non-NULL) plus some offset, at line 302.
Given this analysis, I would rewrite the code as below. This should
not only placate clang's warning, but also make the code faster
rather than slower.
2010-08-30 Bruno Haible <br...@clisp.org>
* lib/hash.c (hash_get_next): Remove unnecessary test against NULL.
Reported by Eric Blake.
*** lib/hash.c.orig Tue Aug 31 01:06:33 2010
--- lib/hash.c Tue Aug 31 01:06:25 2010
***************
*** 307,315 ****
abort ();
/* Find next entry in the same bucket. */
! for (cursor = bucket; cursor; cursor = cursor->next)
! if (cursor->data == entry && cursor->next)
! return cursor->next->data;
/* Find first entry in any subsequent bucket. */
while (++bucket < table->bucket_limit)
--- 307,320 ----
abort ();
/* Find next entry in the same bucket. */
! cursor = bucket;
! do
! {
! if (cursor->data == entry && cursor->next)
! return cursor->next->data;
! cursor = cursor->next;
! }
! while (cursor != NULL);
/* Find first entry in any subsequent bucket. */
while (++bucket < table->bucket_limit)
