Bruno Haible wrote: > 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 <[email protected]> > > * lib/hash.c (hash_get_next): Remove unnecessary test against NULL. > Reported by Eric Blake.
Thanks to both of you. I've pushed your change, Bruno.
