should use PL_DHASH_ENTRY_IS_LIVE instead of PL_DHASH_ENTRY_IS_BUSY

RESOLVED INVALID

Status

RESOLVED INVALID
16 years ago
10 years ago

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Tracking

Trunk
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

16 years ago
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
(Assignee)

Comment 2

16 years ago
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

Comment 4

16 years ago
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
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.