Taking this back to list, in case it collects some more comments.
Context: I sent a PULL request of
git://gitorious.org/erkkise/fdo-xserver.git client-tracking-v2 to Keith,
given that the previous update had not risen any discussion.
On 04.04.2011 21:51, Keith Packard wrote:
On Mon, 04 Apr 2011 12:14:19 +0300, Erkki Seppala<[email protected]>
wrote:
dix: add hashing functions to resource.h for others to use.
CompareResourceID should not be in resource.c
You should not create a new 'HashResourceID' function with a weird
signature. Instead, you should change the function signature of the
existing Hash function and publish that. The new signature should be:
int Hash(XID id, int numBits);
Changed the interface - I still call the function HashResourceID,
though, because it is really usable only for hashing resource IDs.
Internally resource.c now uses the same static function Hash implemented
in terms of HashResourceID.
The signature of my hashing function was intended to be uniform so that
it would work directly with the generic hash table implementation, but
given that it has now moved from dix to Xext, it doesn't seem that
important anymore. So the uniform interface was moved to
Xext/hashtable.c as ht_resourceid_hash/compare.
Your current implementation has HashResourceID returning 'unsigned' with
'0' indicating an error while Hash returns 'int' with -1 indicating an
error. Are you positive that 0 is never a legal hash value? -1 is
clearly OK as numBits is smaller than 31.
The unsigned return value was because it doesn't make sense to me for a
hashing operation to fail. Indeed, the return value is never checked in
resource.c and should the function ever return -1, it will likely cause
random data corruption or crashes. Perhaps it should be an assert instead.
Returning 0 from ht_resourceid_compare just makes the distribution
uneven at worst, so it doesn't matter if the hash value 0 is legal or
not. But I fixed this anyway, because uneven distribution can become a
hidden problem, so now it uses a generic hash function in that situation.
Other updates I did:
Xext/hashtable.c one_at_a_time accepts a const void pointer instead of
of const char pointer
Xext/hashtable.c ht_dump_contents for dumping the contents of the hash
table for debugging purposes
Thanks for the comments! Gitorious is updated.
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
