On 11/17/2011 01:11 PM, Stefan Berger wrote:
> Add a function to the virHashTable for getting an array of the hash table's
> key-value pairs and have the keys (optionally) sorted.
>
> Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>
>
> ---
>
> v5:
> - follwed Eric Blake's suggestions:
> - added better description to new function
> - return array of key-value pairs rather than only keys
Yep, looks like it will be more reusable now. And the more I think
about it, the more I've convinced myself this is a useful interface
(allows you to sort the hashtable in multiple ways, based on what
comparator you pass in; using a data structure to store sorted elements
with hashtable lookup speeds still only gives you a single sort order).
>
> ---
> src/libvirt_private.syms | 2 ++
> src/util/hash.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
> src/util/hash.h | 21 +++++++++++++++++++++
I'd still feel a bit better if we had coverage for the new function in
tests/hashtest.c, but I'll let that slide to another patch; this patch
already blocks enough other useful stuff that it will get some good
field testing from the tck runs you do, while we work on enhancing
hashtest.c.
> }
> +
> +struct getKeysIter
> +{
> + virHashKeyValuePair *sortArray;
> + unsigned arrayIdx;
> +};
Lots simpler, huh? :)
>
> +/*
> + * Get the hash table's key/value pairs and have them optionally sorted.
> + * The returned array contains virHashSize() elements. Aditionally,
s/Aditionally/Additionally/
> + * an empty element has been added to the end of the array whose key == NULL
> + * also indicates the end of the array.
We don't need both "Additionally" and "also" in the same sentence. How
about:
s/also indicates/to indicate/
> + * The key/value pairs are only valid as long as the underlying hash
> + * table is not modified, i.e., keys removed or the hash table deleted.
s/keys removed or the hash table deleted/no keys are removed or
inserted, and the hash table is not deleted/
> + * The caller must only free the returned array using VIR_FREE().
> + * The caller must make copies of all returned keys and values if they are
> + * to be used somewhere else.
> + */
> +typedef struct _virHashKeyValuePair virHashKeyValuePair;
> +typedef virHashKeyValuePair *virHashKeyValuePairPtr;
> +struct _virHashKeyValuePair {
> + const void *key;
> + const void *value;
> +};
> +typedef int (*virHashKeyComparator)(const virHashKeyValuePairPtr ,
> + const virHashKeyValuePairPtr );
The spacing looks awkward on these two lines;
s/ \([,)]\)/\1/
ACK. No need to see a v6 on this one, as the fixes are trivial.
--
Eric Blake eblake@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list