the msg db should use PL_DHASH_ENTRY_IS_LIVE instead of PL_DHASH_ENTRY_IS_BUSY when checking for the presence of a key in the pldhash table. At least, that's Patrick and my reading of pldhash.h - the msg db code was using PL_DHASH_ENTRY_IS_BUSY.
No, the msgdb code is fine, because it uses IS_BUSY only after calling Operate to LOOKUP or ADD, and you'll never get a non-live, busy (i.e., removed) entry back from such a call. If you were enumerating table->entryStore yourself, you'd need IS_LIVE to skip the removed sentinels. But you're not doing that. What misled you in pldhash.h (I'd like to improve the comments)? /be
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → INVALID
Heh, well, Beard was misled by the comments in pldhash.h and spread the infection to me :-) : when I read the comment he pointed me at, I agreed with him. * The PL_DHASH_ENTRY_IS_LIVE macro tests whether entry is neither free nor * removed. An entry may be either busy or free; if busy, it may be live or * removed. Consumers of this API should not access members of entries that * are not live. I guess you could add something like the comment above about enumerating table->entryStore yourself. Is it the case that the API's will never give you non-live, busy entry? The API's being ENUMERATE and OPERATE. Or could you run into trouble if you actually held on to PLDHashEntryHdr *'s? (that would seem a bit risky...)
You can't hold onto an entry pointer safely, because the table may grow or shrink (you can optimistically hold onto an entry pointer, but only if you also hold a sample of table->generation, and dereference the entry pointer iff the sample matches the current table->generation). You won't get called on removed entries from Enumerate, and Operate won't return them as I wrote. I'll add a note to the comment -- thanks for that feedback. /be
Still, this begs the question, is it better "style" to use PL_DHASH_ENTRY_IS_BUSY or PL_DHASH_ENTRY_IS_LIVE?
BUSY is better, it's cheaper on some architectures to test equality or inequality with zero than to test a relational like >= 2. /be
Created attachment 102731 [details] [diff] [review] comment improvements I'm checking in these without r/sr/a=. /be
You need to log in before you can comment on or make changes to this bug.