Closed Bug 1376498 Opened 2 years ago Closed 2 years ago

Add nsInterfaceHashtable::Remove and make other hashtable Remove methods consistent

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: mats, Assigned: mats)

Details

Attachments

(5 files)

Currently you have to do something like this:

nsCOMPtr<nsISomething> value;
if (auto entry = hashtable.Lookup(key)) {
  value = entry.Data().forget();
  entry.Remove();
  ...
}

which is a bit klunky.  This would be simpler:

nsCOMPtr<nsISomething> value;
if (hashtable.Remove(key, getter_AddRefs(value))) {
  ...
}
Assignee: nobody → mats
Summary: Consider adding nsInterfaceHashtable::Remove → Add nsInterfaceHashtable::Remove and make other hashtable Remove methods consistent
(nsClassHashtable comes in a later part)
Attachment #8882815 - Flags: review?(nfroyd)
A note about the first hunk in part 4: there's a minor semantic change there -
now checking for the existence of the entry rather than non-nullness of
its value, but I checked that null is never added to this hashtable:
http://searchfox.org/mozilla-central/rev/dde5c480358718607cc40d752656c968a0e6eabd/dom/base/CustomElementRegistry.cpp#299-300
so I figured it was worth it to make the code more idiomatic.
I added a MOZ_ASSERT(value) to make that invariant explicit.
From an API consistency POV, this makes nsDataHashtable::GetAndRemove
an aberration:
http://searchfox.org/mozilla-central/rev/dde5c480358718607cc40d752656c968a0e6eabd/xpcom/ds/nsDataHashtable.h#47

There's very few uses of that:
http://searchfox.org/mozilla-central/search?q=symbol:_ZN15nsDataHashtable12GetAndRemoveENS_7KeyTypeE&redirect=false
so I'm leaning towards converting those to also use Remove so that
we uniformly use the same Remove API everywhere.

WDYT?
... alternatively, we could add GetAndRemove to the other hashtable
classes too, so that it's consistently available everywhere or not
at all.  It's slightly more ergonomic than Remove() in some cases. e.g.

  return stylesheets.GetAndRemove(raw).valueOr(nullptr);

vs.

  ServoStyleSheet* sheet;
  stylesheets.Remove(raw, &sheet);
  return sheet;
... when I look through the existing uses of GetAndRemove though,
the one mentioned above is the only one that's motivated.  The other
ones can easily use Remove instead.  So perhaps it's not worth it
to have two ways to remove something for this one case?
Comment on attachment 8882815 [details] [diff] [review]
Unify the ns[Base|Interface|RefPtr]Hashtable::Remove() signatures for consistency

Review of attachment 8882815 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but I want to double-check the rationale on the not-found behavior for Remove().

::: xpcom/ds/nsBaseHashtable.h
@@ +159,4 @@
>     * @param aKey the key to remove from the hashtable
> +   * @param aData where to move the value (if non-null).  If an entry is not
> +   *              found, *aData will be assigned a default-constructed value
> +   *              (i.e. reset to zero or nullptr for primitive types).

This seems like a really weird contract; I would expect that *aData would not be touched if false was returned.  I guess it saves a branch in some cases?
Attachment #8882815 - Flags: review?(nfroyd) → review+
Attachment #8882816 - Flags: review?(nfroyd) → review+
Attachment #8882818 - Flags: review?(nfroyd) → review+
Attachment #8882819 - Flags: review?(nfroyd) → review+
Attachment #8882821 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #11)
> This seems like a really weird contract; I would expect that *aData would
> not be touched if false was returned.

I agree, but I wanted to be consistent with the existing contract for
ns[RefPtr|Class]Hashtable::Remove which both assign null:
http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/xpcom/ds/nsRefPtrHashtable.h#187
same as the Get method does when an entry does not exist:
http://searchfox.org/mozilla-central/rev/a3a739de04ee6134c11546568a33dbb6a6a29907/xpcom/ds/nsRefPtrHashtable.h#116

Not assigning the outparam is fine with me, but we probably need to
do some auditing before changing those methods.  (Let me know if you
want me to file a follow-up bug on that.)

> I guess it saves a branch in some cases?

I doubt it matters for performance since these calls are inlined.
The vast majority of calls with an 'aData' argument use
getter_AddRefs(localVar) or &localVar so the assignment can
probably be optimized away (as redundant) in many cases, or if
the local variable isn't used after Remove/Get returns false
then the assignment can be optimized away for that reason.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ed4185c7b73
part 1 - Unify the ns[Base|Interface|RefPtr]Hashtable::Remove() signatures for consistency.  Make it return true if an entry was removed, with an optional out param to move the value.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/d46d1d8506ad
part 2 - Use plain Remove(key) in some places instead of Lookup(key).Remove() for simplicity.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/29730c843219
part 3 - Make nsClassHashtable inherit Remove() for API consistency with ns[Base|Interface|RefPtr]Hashtable.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/512c69e35fc0
part 4 - Make all nsClassHashtable::RemoveAndForget() consumers use Remove() instead.  r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4a27ab70450
part 5 - Remove nsClassHashtable::RemoveAndForget().  r=froydnj
You need to log in before you can comment on or make changes to this bug.