[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH V5 01/10] Add function to get hash tables key/value pairs



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 linux vnet ibm com>
> 
> ---
> 
> 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 redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]