The other day in the #content
IRC channel:
<bz> I have learned so many things about how to not define APIs in my work with Mozilla code ;) <bz> (probably lots more to learn, though)
I, too, am still learning a lot about what makes a good API. Like a lot of other things, it’s easier to point out poor API design than to describe examples of good API design, and that’s what this blog post is about. In particular, the venerable XPCOM datastructure PLDHashTable
has been undergoing a number of changes lately, all aimed at bringing it up to date. (The question of why we have our own implementations of things that exist in the C++ standard library is for a separate blog post.)
The whole effort started with noticing that PL_DHashTableOperate
is not a well-structured API. It’s necessary to quote some more of the API surface to fully understand what’s going on here:
typedef enum PLDHashOperator { PL_DHASH_LOOKUP = 0, /* lookup entry */ PL_DHASH_ADD = 1, /* add entry */ PL_DHASH_REMOVE = 2, /* remove entry, or enumerator says remove */ PL_DHASH_NEXT = 0, /* enumerator says continue */ PL_DHASH_STOP = 1 /* enumerator says stop */ } PLDHashOperator; typedef PLDHashOperator (* PLDHashEnumerator)(PLDHashTable *table, PLDHashEntryHdr *hdr, uint32_t number, void *arg); uint32_t PL_DHashTableEnumerate(PLDHashTable *table, PLDHashEnumerator etor, void *arg); PLDHashEntryHdr* PL_DHashTableOperate(PLDHashTable* table, const void* key, PLDHashOperator op);
(PL_DHashTableOperate
no longer exists in the tree due to other cleanup bugs; the above is approximately what it looked like at the end of 2014.)
There are several problems with the above slice of the API:
PL_DHashTableOperate(table, key, PL_DHASH_ADD)
is a long way to spell what should have been namedPL_DHashTableAdd(table, key)
- There’s another problem with the above: it’s making a runtime decision (based on the value of
op
) about what should have been a compile-time decision: this particular call will always and forever be an add operation. We shouldn’t have the (admittedly small) runtime overhead of dispatching onop
. It’s worth noting that compiling with LTO and a quality inliner will remove that runtime overhead, but we might as well structure the code so non-LTO compiles benefit and the code at callsites reads better. - Given the above definitions, you can say
PL_DHashTableOperate(table, key, PL_DHASH_STOP)
and nothing will complain. ThePL_DHASH_NEXT
andPL_DHASH_STOP
values are really only for a function of typePLDHashEnumerator
to return, but nothing about the above definition enforces that in any way. Similarly, you can returnPL_DHASH_LOOKUP
from aPLDHashEnumerator
function, which is nonsensical. - The requirement to always return a
PLDHashEntryHdr*
fromPL_DHashTableOperate
means doing aPL_DHASH_REMOVE
has to return something; it happens to returnnullptr
always, but it really should returnvoid
. In a similar fashion,PL_DHASH_LOOKUP
always returns a non-nullptr
pointer (!); one has to checkPL_DHASH_ENTRY_IS_{FREE,BUSY}
on the returned value. The typical style for an API like this would be to returnnullptr
if an entry for the given key didn’t exist, and a non-nullptr
pointer if such an entry did. The free-ness or busy-ness of a given entry should be a property entirely internal to the hashtable implementation (it’s possible that some scenarios could be slightly more efficient with direct access to the busy-ness of an entry).
We might infer corresponding properties of a good API from each of the above issues:
- Entry points for the API produce readable code.
- The API doesn’t enforce unnecessary overhead.
- The API makes it impossible to talk about nonsensical things.
- It is always reasonably clear what return values from API functions describe.
Fixing the first two bulleted issues, above, was the subject of bug 1118024, done by Michael Pruett. Once that was done, we really didn’t need PL_DHashTableOperate
, and removing PL_DHashTableOperate
and related code was done in bug 1121202 and bug 1124920 by Michael Pruett and Nicholas Nethercote, respectively. Fixing the unusual return convention of PL_DHashTableLookup
is being done in bug 1124973 by Nicholas Nethercote. Maybe once all this gets done, we can move away from C-style PL_DHashTable*
functions to C++ methods on PLDHashTable
itself!
Next time we’ll talk about the actual contents of a PL_DHashTable
and how improvements have been made there, too.