149785 - nsHashTable hides important functionality of PLHashTable

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Conrad Carlen (not reading bugmail)]

With the enumeration callback of nsHashTable you can only return PR_TRUE to
continue iteration or PR_FALSE to stop it. One of the more useful things about
PLHashTable is that you can return a flag to cause the current item to be removed.

Comment 1

[Conrad Carlen (not reading bugmail)]

Attachment: patch to allow removal

Because a PRBool is the same as a PRIntn and because of the values of the enum
return values, existing callers which return PR_TRUE or PR_FALSE can go
unchanged.

Comment 2

[Conrad Carlen (not reading bugmail)]

Noting blockage. In the next revision for the patch to bug 70714, I need to be
able to enumerate an nsHashTable and remove certain entries from it.

Brendan, can you review?

Comment 3

[Alec Flett]

Comment on attachment 86736 [details] [diff] [review]
patch to allow removal

I like the way this is done - it also catches lame things like if someone
writes "return NS_ERROR_FAILURE" (Because they thought it was a boolean) it
will still fall back to HT_ENUMERATE_STOP.

sr=alecf
Lets get dougt to review this too.

Comment 4

[Alec Flett]

doug, you wanna take a look at this? very useful, and we need it for bug 70714

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 86736 [details] [diff] [review]
patch to allow removal

>Index: nsHashtable.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpcom/ds/nsHashtable.cpp,v
>retrieving revision 3.56
>diff -w -u -4 -r3.56 nsHashtable.cpp
>--- nsHashtable.cpp	15 May 2002 18:55:09 -0000	3.56
>+++ nsHashtable.cpp	7 Jun 2002 00:03:07 -0000
>@@ -193,11 +193,17 @@
> 
> static PRIntn PR_CALLBACK _hashEnumerate(PLHashEntry *he, PRIntn i, void *arg)
> {
>     _HashEnumerateArgs* thunk = (_HashEnumerateArgs*)arg;
>-    return thunk->fn((nsHashKey *) he->key, he->value, thunk->arg)
>-           ? HT_ENUMERATE_NEXT
>-           : HT_ENUMERATE_STOP;
>+    switch(thunk->fn((nsHashKey *) he->key, he->value, thunk->arg)) {

Nit: prevailing style puts a space after switch, if, while, etc. and before the
(.

>+        case kHashEnumerateNext:
>+            return HT_ENUMERATE_NEXT;

Nit: the case label is not a statement so don't (over-)indent it.  At most,
half-indent it.  In any case, the return should be one indentation level
(c-basic-offset) in from the switch keyword.

r=[e-mail] with these picked.

/be

Comment 6

[Conrad Carlen (not reading bugmail)]

Thanks. I'll fix up the spacing issues brendan pointed out & seek approval to
get this in 1.1beta.

Comment 7

[Asa Dotzler [:asa]]

Comment on attachment 86736 [details] [diff] [review]
patch to allow removal

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 8

[Alec Flett]

any chance we can get this landed for 1.1? I noticed it got approval, but I
suppose that approval has expired at this point..

Comment 9

[Brendan Eich [:brendan]]

What happened?  I can approve again for 1.1 checkin, today -- drivers agree. 
Someone please do it fast.

/be

Comment 10

[Conrad Carlen (not reading bugmail)]

Checked in.